Skip to content

cbo should be enabled on other platforms to support ZTP #189

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 5 commits into from
Oct 27, 2021
Merged

cbo should be enabled on other platforms to support ZTP #189

openshift-merge-robot merged 5 commits into from
Oct 27, 2021

Conversation

@asalkeld
Copy link
Contributor

@asalkeld asalkeld commented Aug 3, 2021 •
edited
Loading

/cc @sadasu
enhancement is now merged.

@openshift-ci openshift-ci bot requested a review from sadasu August 3, 2021 22:22
@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asalkeld

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2021
@romfreiman
Copy link

romfreiman commented Aug 4, 2021 •
edited
Loading

few comments:

  1. Lets try to understand whether OpenStack is a requirement
  2. if CBO is enabled, should it get the manifests for workload partitioning (see here: https://github.com/openshift/enhancements/blob/master/enhancements/workload-partitioning/management-workload-partitioning.md#example-manifests) cc @dhellmann
  3. Will it affect SNO performance? cc: @browsell

@hardys

@hardys
Copy link

hardys commented Aug 4, 2021 •
edited
Loading

I'm not sure if this is the right approach tbh - it means that in theory any UPI deployment can enable metal3 deployments, right?

Also as @romfreiman says we're not sure if this will be sufficient, as we know there have been requests to run the metal3 components on openstack (and vsphere IIRC).

It's tricky - on the one hand we want to enable flexibility so folks can "opt in" to CBO and thus metal3 components on platforms other than baremetal, but how do we constrain the test/support matrix to platforms we actually validate as working?

We're past FF at this point, so I'd suggest we have some more detailed discussion in an enhancement proposal, so we can clarify these details before executing the plan for the next dev cycle?

@hardys hardys removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2021
@asalkeld asalkeld changed the title cbo should be enabled on baremetal and "none" platforms cbo should be enabled on other platforms to support ZTP Aug 20, 2021
@asalkeld asalkeld mentioned this pull request Aug 20, 2021
Merged
@asalkeld asalkeld force-pushed the enable-cbo-on-all-platforms branch from 3c25bf5 to e8ee0b7 Compare August 20, 2021 02:09
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021
@asalkeld
Copy link
Contributor Author

asalkeld commented Aug 26, 2021

/retest

@asalkeld asalkeld force-pushed the enable-cbo-on-all-platforms branch from 9a14f78 to fc18ab8 Compare September 14, 2021 05:10
@sadasu
Copy link
Contributor

sadasu commented Sep 23, 2021

Do these changes also include disabling metal3 when the Topology is "External"? https://github.com/openshift/enhancements/pull/871/files#diff-7fe1b077c80e38c1b2725c8fcd5cf98a3049943033fa28a25e51cdd4b22a427cR93

@asalkeld
Copy link
Contributor Author

asalkeld commented Sep 27, 2021 •
edited
Loading

Do these changes also include disabling metal3 when the Topology is "External"? https://github.com/openshift/enhancements/pull/871/files#diff-7fe1b077c80e38c1b2725c8fcd5cf98a3049943033fa28a25e51cdd4b22a427cR93

@sadasu Yes, they do now.

@asalkeld asalkeld force-pushed the enable-cbo-on-all-platforms branch from fc18ab8 to da8b35c Compare October 13, 2021 23:57
@asalkeld
Copy link
Contributor Author

asalkeld commented Oct 13, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2021
@asalkeld
Copy link
Contributor Author

asalkeld commented Oct 14, 2021

/retest

@dhellmann
Copy link
Contributor

dhellmann commented Oct 14, 2021

/uncc

sadasu
sadasu reviewed Oct 19, 2021
View reviewed changes
controllers/provisioning_controller.go
}
// Temporarily not requeuing request
return ctrl.Result{}, nil
// Always re-validate the provisioning configuration is valid.
Copy link
Contributor

@sadasu sadasu Oct 19, 2021

Choose a reason for hiding this comment

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

Good point.

@sadasu
Copy link
Contributor

sadasu commented Oct 19, 2021

IsEnabled() could return false if the Platform or the Topology are unsupported. So, could we modify da8b35c#diff-7ad72d08a06fe28b03a15c84289b815f693443562d0c67e6f461c7e3f5ee0707R467 to reflect that?

sadasu
sadasu reviewed Oct 19, 2021
View reviewed changes
controllers/provisioning_enabled.go
}
}

func EnabledFeatures(ctx context.Context, osClient osclientset.Interface) (v1alpha1.EnabledFeatures, error) {
Copy link
Contributor

@sadasu sadasu Oct 19, 2021

Choose a reason for hiding this comment

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

Both these methods are reading the same resources. Even though they are read from the cache, we could make it more efficient by setting EnabledFeatures while determining if this is a supported platform.

sadasu
sadasu reviewed Oct 19, 2021
View reviewed changes
main.go
ReleaseVersion: releaseVersion,
ImagesFilename: imagesJSONFilename,
WebHookEnabled: enableWebhook,
EnabledFeatures: enabledFeatures,
Copy link
Contributor

@sadasu sadasu Oct 19, 2021

Choose a reason for hiding this comment

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

We can determine enabledFeatures while calling IsEnabled() in SetupWithManager().

@asalkeld
Update openshift/api to get the latest Infrastructure definition
e584929
@asalkeld
CRD formatting changes from module update
913d8b1
@asalkeld
Always re-validate the provisioning configuration is valid
617d99c
@asalkeld
cbo should support ZTP on the platforms None, OpenShift and vSphere
973e875 
- ZTP uses provisioningNetwork=Disabled so allow this and nothing else on
  these platforms to reduce support/testing load.
- Make sure we are disabled when infra.Status.ControlPlaneTopology == osconfigv1.ExternalTopologyMode
@asalkeld
Reduce the duplication in the provisioning_enabled.go file
ad1fc24
@asalkeld asalkeld force-pushed the enable-cbo-on-all-platforms branch from da8b35c to ad1fc24 Compare October 19, 2021 23:14
@asalkeld
Copy link
Contributor Author

asalkeld commented Oct 20, 2021

/retest

1 similar comment
@asalkeld
Copy link
Contributor Author

asalkeld commented Oct 20, 2021

/retest

@sadasu
Copy link
Contributor

sadasu commented Oct 27, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2021
@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

openshift-bot commented Oct 27, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 7bfc0bf into openshift:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sadasu sadasu sadasu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sadasu sadasu

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@asalkeld @romfreiman @hardys @sadasu @dhellmann @openshift-bot @openshift-merge-robot