OCPEDGE-1740: feat: add arbiter role support to ABI #9780
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@eggfoobar: This pull request references OCPEDGE-1740 which is a valid jira issue. In response to this:
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. |
pkg/asset/agent/installconfig.go
Outdated
| @@ -88,14 +88,15 @@ func (a *OptionalInstallConfig) validateInstallConfig(ctx context.Context, insta | |||
| allErrs = append(allErrs, err...) | |||
| } | |||
|
|
|||
| if installConfig.FeatureSet != configv1.Default { | |||
| // TODO: Remove arbiter check once arbiter hits GA. | |||
| if installConfig.FeatureSet != configv1.Default && !installConfig.IsArbiterEnabled() { | |||
There was a problem hiding this comment.
Hey @zaneb and @rwsu, I know we had briefly mentioned resolving this check (https://issues.redhat.com/browse/AGENT-554), but I couldn't find where the agent flow would be using this to confidently remove this check, currently the assisted service does not utilize it for arbiter and is adding the CustomNoUpgrade featureSet on their end https://github.com/giladravid16/assisted-service/blob/master/internal/installcfg/builder/builder.go#L153
Any concerns with this quick approach?
There was a problem hiding this comment.
This seems OK (though it would be better to have AGENT-554 implemented). Do we already have a validation that if IsArbiterEnabled() is true then the FeatureSet must be CustomNoUpgrade? (Presumably yes; that's why assisted-service is setting it. So I guess the real question is if that validation runs in the ABI path.)
FWIW the thing that needs to happen for AGENT-554 is to add the FeatureSet to the install-config overrides here, which wouldn't be difficult.
There was a problem hiding this comment.
Oh, if it's just that, then I went ahead and made the update, it should match correctly with the de-serielizaed step on the assisted-service install cfg struct
|
/retest-required |
|
/retest |
| @@ -40,6 +40,22 @@ num_known_hosts() { | |||
| done | |||
| echo "Hosts known and ready for cluster installation (${known_hosts}/${total_required_nodes})" 1>&2 | |||
| fi | |||
| if (( known_hosts > total_required_nodes )) && [[ "${TNA_CLUSTERS_SUPPORT}" == "true" ]]; then | |||
There was a problem hiding this comment.
I would suggest adding a REQUIRED_ARBITER_NODES variable to https://github.com/openshift/installer/blob/main/data/data/agent/files/usr/local/share/start-cluster/start-cluster.env.template. That could simplify or eliminate this code block. The required master and workers are calculated here and passed to the template:
installer/pkg/asset/agent/image/ignition.go
Line 180 in 55b3518
There was a problem hiding this comment.
AgentClusterInstall is missing the arbiter role: https://github.com/openshift/assisted-service/blob/dafbf3354b6e2f587491ba3a96b352d63abebfbe/api/hiveextension/v1beta1/agentclusterinstall_types.go#L365
There was a problem hiding this comment.
Oh sure thing, way better approach, I got a PR open here openshift/assisted-service#7765
There was a problem hiding this comment.
Added the change for now, until that PR is merged in, then I'll update the go.mod file
|
Per Richard's comment, would also need to add a validation to pkg/asset/agent/manifests/agentclusterinstall.go when the number of arbiters is added to AgentClusterInstall. |
eggfoobar
force-pushed
the
support-arbiter-agent-based
branch
from
80974b4 to
f997529
Compare
|
/hold Holding because I hard coded modified the vendor agent cluster install config to test out install until openshift/assisted-service#7765 gets merged in. |
openshift-ci
bot
added
the
do-not-merge/hold
eggfoobar
force-pushed
the
support-arbiter-agent-based
branch
from
f997529 to
36f8417
Compare
openshift-merge-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
added support for arbiter installs to ABI flow, we currently do not support installing TechPreview featureSet with agent based install, this includes adding that capability for overriding featureSet to be passed to the assisted service. Signed-off-by: ehila <[email protected]>
added arbiter count passing through annotation for testing, will remove before merge Signed-off-by: ehila <[email protected]>
eggfoobar
force-pushed
the
support-arbiter-agent-based
branch
from
61d16fd to
f58624d
Compare
openshift-merge-robot
removed
the
needs-rebase
Signed-off-by: ehila <[email protected]>
|
@eggfoobar: The following tests failed, say
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. |
Assisted service now supports the arbiter node, most of the folks looking to use the arbiter node have a low tolerance for extra compute nodes, as such a bootstrap node is not ideal. This PR adds support for the arbiter node to the ABI flow using the latest assisted service additions.
Added support for arbiter installs to ABI flow, we currently do not support installing TechPreview featureSet with agent based install so a temporary override was added for arbiter due to customers who have limitations on running a bootstrap node and will need to run TechPreview installs.