Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPEDGE-1311: add two node openshift deployment #7321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qJkee
Copy link

@qJkee qJkee commented Feb 17, 2025

Add new deployment type to the assisted service called two node with arbiter(TNA). Install config changes are made according to openshift/enhancements#1674 proposal

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see [CONTRIBUTING] guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

Add new deployment type to the assisted service
called two node with arbiter(TNA)
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 17, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 17, 2025

@qJkee: This pull request references OCPEDGE-1311 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Add new deployment type to the assisted service called two node with arbiter(TNA). Install config changes are made according to openshift/enhancements#1674 proposal

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see [CONTRIBUTING] guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 17, 2025
@openshift-ci openshift-ci bot added the api-review Categorizes an issue or PR as actively needing an API review. label Feb 17, 2025
Copy link

openshift-ci bot commented Feb 17, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qJkee
Once this PR has been reviewed and has the lgtm label, please assign javipolo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 67.95%. Comparing base (f06b712) to head (c5dc4ec).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
internal/installcfg/builder/builder.go 0.00% 8 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7321      +/-   ##
==========================================
- Coverage   67.96%   67.95%   -0.02%     
==========================================
  Files         300      300              
  Lines       41007    41023      +16     
==========================================
+ Hits        27872    27878       +6     
- Misses      10632    10641       +9     
- Partials     2503     2504       +1     
Files with missing lines Coverage Δ
internal/installcfg/builder/builder.go 75.71% <0.00%> (-3.40%) ⬇️

... and 2 files with indirect coverage changes

@carbonin
Copy link
Member

Are we additionally expected to deploy the arbiter node? How is that being handled?

@@ -112,6 +112,18 @@ func (i *installConfigBuilder) getBasicInstallConfig(cluster *common.Cluster) (*
SSHKey: cluster.SSHPublicKey,
}

if cfg.ControlPlane.Replicas == 2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will we differentiate between the arbiter case and the two node with fencing case once that comes through?

@@ -41,14 +41,17 @@ const (

// HostRoleBootstrap captures enum value "bootstrap"
HostRoleBootstrap HostRole = "bootstrap"

// HostRoleArbiter captures enum value "arbiter"
HostRoleArbiter HostRole = "arbiter"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this role get assigned?

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading more of the enhancement it does seem like we'll need significantly more changes to support this.

We'll need to update host minimum requirements for the arbiter role, we'll need to support reading the new ignition from the installer and ensuring the host designated as the arbiter downloads it, and we'll probably also need some way to ensure this is gated to only openshift versions that support TNA

Copy link

openshift-ci bot commented Feb 17, 2025

@qJkee: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/edge-lint c5dc4ec link true /test edge-lint
ci/prow/edge-verify-generated-code c5dc4ec link true /test edge-verify-generated-code
ci/prow/okd-scos-e2e-aws-ovn c5dc4ec link false /test okd-scos-e2e-aws-ovn

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants