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

OTA-1010: release extract: --include works for a minor level update #1954

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

Conversation

hongkailiu
Copy link
Member

@hongkailiu hongkailiu commented Dec 12, 2024

This is still a patch on the extract command.
Comparing the the current solution that disables the net-new capabilities by default and enabled a few that handles the 4.13-4.14 upgrade only, the current patch enables the net-new capabilities by default.
The advantage is that it avoids enumeration of growing releases but the disadvantage is that it might exclude expected manifests.

The ideal solution is to compute the exact set of capabilities after upgrading to the incoming release but it would involve significant changes on the code. We plan to develop some functions in the go-lib that can be shared by oc-cli and cvo.

/cc @wking @petr-muller

@openshift-ci openshift-ci bot requested review from petr-muller and wking December 12, 2024 20:06
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 12, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 12, 2024

@hongkailiu: This pull request references OTA-1010 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:

/hold

Requiring openshift/library-go#1908

/cc @wking @petr-muller

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 do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2024
@hongkailiu hongkailiu force-pushed the OTA-1010 branch 12 times, most recently from 34cce22 to 8d7e2da Compare December 14, 2024 16:31
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 6, 2025

@hongkailiu: This pull request references OTA-1010 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:

This is still a patch on the extract command.
Comparing the the current solution that disables the net-new capabilities by default and enabled a few that handles the 4.13-4.14 upgrade only, the current patch enables the net-new capabilities by default.
The advantage is that it avoids enumeration of growing releases but the disadvantage is that it might exclude expected manifests.

The ideal solution is to compute the exact set of capabilities after upgrading to the incoming release but it would involve significant changes on the code. We plan to develop some functions in the go-lib that can be shared by oc-cli and cvo.

/cc @wking @petr-muller

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.

@hongkailiu
Copy link
Member Author

/retest-required

@hongkailiu hongkailiu force-pushed the OTA-1010 branch 2 times, most recently from 8a0072b to 959afab Compare January 6, 2025 21:16
@hongkailiu
Copy link
Member Author

/retest-required

@hongkailiu hongkailiu force-pushed the OTA-1010 branch 2 times, most recently from cff3929 to cbee2c7 Compare January 6, 2025 22:08
Copy link
Contributor

openshift-ci bot commented Jan 6, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hongkailiu
Once this PR has been reviewed and has the lgtm label, please assign wking 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


key := configv1.ClusterVersionCapabilitySetCurrent
if clusterVersion.Spec.Capabilities != nil && clusterVersion.Spec.Capabilities.BaselineCapabilitySet != "" {
key = clusterVersion.Spec.Capabilities.BaselineCapabilitySet
Copy link
Member

@wking wking Jan 6, 2025

Choose a reason for hiding this comment

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

You should use config.Capabilities here (set from &clusterVersion.Status.Capabilities a few lines up), to avoid having to worry about the pre-CVO-verification Spec content. [edit: no you're right as you have it, because only spec will include baselineCapabilitySet].

Copy link
Member Author

Choose a reason for hiding this comment

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

"vCurrent" might grow for a minor level upgrade. We want to make sure it has all enabled from the BaselineCapabilitySet for the new version.

if clusterVersion.Spec.Capabilities != nil && clusterVersion.Spec.Capabilities.BaselineCapabilitySet != "" {
key = clusterVersion.Spec.Capabilities.BaselineCapabilitySet
}
enabled := sets.New[configv1.ClusterVersionCapability](configv1.ClusterVersionCapabilitySets[key]...)
Copy link
Member

Choose a reason for hiding this comment

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

You'll also want to Insert config.Capabilities.AdditionalEnabledCapabilities in the enabled set.

Copy link
Member Author

Choose a reason for hiding this comment

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

clusterVersion.Spec.Capabilities.AdditionalEnabledCapabilities is a list of capabilities specified by the admin.
They should be included already in cv.status.Capabilities.EnabledCapabilities.

for _, s := range configv1.ClusterVersionCapabilitySets {
known.Insert(s...)
}
previouslyKnown := sets.New[configv1.ClusterVersionCapability](config.Capabilities.KnownCapabilities...)
Copy link
Member

Choose a reason for hiding this comment

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

isn't previouslyKnown just config.Capabilities.KnownCapabilities, frozen out from the cluster's current ClusterVersion status?

Copy link
Member Author

@hongkailiu hongkailiu Jan 7, 2025

Choose a reason for hiding this comment

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

Yes, it is. We just convert it to a set which is used to calculate other sets below such as deltaKnown.
Maybe I did not get your question?

Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

I only skimmed the code without focusing too much on change correctness, leaving that for Trevor b/c he's already involved here.

pkg/cli/admin/release/extract.go Outdated Show resolved Hide resolved
pkg/cli/admin/release/extract.go Outdated Show resolved Hide resolved
pkg/cli/admin/release/extract.go Show resolved Hide resolved
pkg/cli/admin/release/extract.go Outdated Show resolved Hide resolved
pkg/cli/admin/release/extract_tools.go Show resolved Hide resolved
@hongkailiu
Copy link
Member Author

/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 Jan 9, 2025
@hongkailiu
Copy link
Member Author

/retest-required

3 similar comments
@hongkailiu
Copy link
Member Author

/retest-required

@hongkailiu
Copy link
Member Author

/retest-required

@hongkailiu
Copy link
Member Author

/retest-required

Copy link
Contributor

openshift-ci bot commented Jan 15, 2025

@hongkailiu: 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/e2e-metal-ipi-ovn-ipv6 a8e41fe link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-ovn-serial a8e41fe link true /test e2e-aws-ovn-serial

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
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants