Skip to content

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

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

Closed
wants to merge 1 commit into from
+270 −43
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions pkg/cli/admin/release/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,20 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/cli-runtime/pkg/genericiooptions"
appsv1client "k8s.io/client-go/kubernetes/typed/apps/v1"
"k8s.io/client-go/rest"
kcmdutil "k8s.io/kubectl/pkg/cmd/util"
"k8s.io/kubectl/pkg/util/templates"

imagev1 "github.com/openshift/api/image/v1"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
"github.com/openshift/library-go/pkg/image/dockerv1client"
"github.com/openshift/library-go/pkg/manifest"
"github.com/openshift/oc/pkg/cli/image/extract"
"github.com/openshift/oc/pkg/cli/image/imagesource"
imagemanifest "github.com/openshift/oc/pkg/cli/image/manifest"
"github.com/openshift/oc/pkg/cli/image/workqueue"
"github.com/openshift/oc/pkg/version"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -94,7 +97,8 @@ func NewExtract(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.C
If --install-config is set, it will be used to determine the expected cluster configuration,
otherwise the command will interrogate your current cluster to determine its configuration.
This command is most accurate when the version of the extracting client matches the version
of the cluster under consideration.
of the cluster under consideration. Otherwise, for example, newly introduced capabilities in
the version of the extracting client might be considered enabled.

Instead of extracting the manifests, you can specify --git=DIR to perform a Git
checkout of the source code that comprises the release. A warning will be printed
Expand Down Expand Up @@ -359,10 +363,27 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
if o.Included {
context := "connected cluster"
inclusionConfig := manifestInclusionConfiguration{}

clientVersion, reportedVersion, versionErr := version.ExtractVersion()
if versionErr != nil {
return versionErr
}
if reportedVersion == "" {
reportedVersion = clientVersion.String()
}

if o.InstallConfig == "" {
inclusionConfig, err = findClusterIncludeConfig(ctx, o.RESTConfig)
configv1client, configv1clientErr := configv1client.NewForConfig(o.RESTConfig)
if configv1clientErr != nil {
return configv1clientErr
}
appsv1client, appsv1clientErr := appsv1client.NewForConfig(o.RESTConfig)
if appsv1clientErr != nil {
return appsv1clientErr
}
inclusionConfig, err = findClusterIncludeConfig(ctx, configv1client, appsv1client, reportedVersion)
} else {
inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig)
inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig, reportedVersion)
context = o.InstallConfig
}
if err != nil {
Expand Down
71 changes: 31 additions & 40 deletions pkg/cli/admin/release/extract_tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"io"
"os"
"path/filepath"
"regexp"
"runtime"
"sort"
"strings"
Expand All @@ -32,7 +31,6 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/cli-runtime/pkg/genericiooptions"
appsv1client "k8s.io/client-go/kubernetes/typed/apps/v1"
"k8s.io/client-go/rest"
"sigs.k8s.io/yaml"

"github.com/MakeNowJust/heredoc"
Expand All @@ -43,7 +41,6 @@ import (
"github.com/openshift/oc/pkg/cli/admin/internal/codesign"
"github.com/openshift/oc/pkg/cli/image/extract"
"github.com/openshift/oc/pkg/cli/image/imagesource"
"github.com/openshift/oc/pkg/version"
)

// extractTarget describes how a file in the release image can be extracted to disk.
Expand Down Expand Up @@ -1167,17 +1164,9 @@ func copyAndReplace(errorOutput io.Writer, w io.Writer, r io.Reader, bufferSize

}

func findClusterIncludeConfigFromInstallConfig(ctx context.Context, installConfigPath string) (manifestInclusionConfiguration, error) {
func findClusterIncludeConfigFromInstallConfig(ctx context.Context, installConfigPath, ocVersion string) (manifestInclusionConfiguration, error) {
config := manifestInclusionConfiguration{}

clientVersion, reportedVersion, err := version.ExtractVersion()
if err != nil {
return config, err
}
if reportedVersion == "" {
reportedVersion = clientVersion.String()
}

installConfigBytes, err := os.ReadFile(installConfigPath)
if err != nil {
return config, err
Expand Down Expand Up @@ -1205,67 +1194,69 @@ func findClusterIncludeConfigFromInstallConfig(ctx context.Context, installConfi
return config, fmt.Errorf("unrecognized baselineCapabilitySet %q", data.Capabilities.BaselineCapabilitySet)
} else {
if data.Capabilities.BaselineCapabilitySet == configv1.ClusterVersionCapabilitySetCurrent {
klog.Infof("If the eventual cluster will not be the same minor version as this %s 'oc', the actual %s capability set may differ.", reportedVersion, data.Capabilities.BaselineCapabilitySet)
klog.Infof("If the eventual cluster will not be the same minor version as this %s 'oc', the actual %s capability set may differ.", ocVersion, data.Capabilities.BaselineCapabilitySet)
}
config.Capabilities.EnabledCapabilities = append(config.Capabilities.EnabledCapabilities, enabled...)
}
config.Capabilities.EnabledCapabilities = append(config.Capabilities.EnabledCapabilities, data.Capabilities.AdditionalEnabledCapabilities...)

klog.Infof("If the eventual cluster will not be the same minor version as this %s 'oc', the known capability sets may differ.", reportedVersion)
klog.Infof("If the eventual cluster will not be the same minor version as this %s 'oc', the known capability sets may differ.", ocVersion)
config.Capabilities.KnownCapabilities = configv1.KnownClusterVersionCapabilities
}

return config, nil
}

func findClusterIncludeConfig(ctx context.Context, restConfig *rest.Config) (manifestInclusionConfiguration, error) {
func findClusterIncludeConfig(ctx context.Context, configv1client configv1client.ConfigV1Interface, appsv1client appsv1client.AppsV1Interface, ocVersion string) (manifestInclusionConfiguration, error) {
config := manifestInclusionConfiguration{}

client, err := configv1client.NewForConfig(restConfig)
if err != nil {
return config, err
}

if featureGate, err := client.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}); err != nil {
if featureGate, err := configv1client.FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}); err != nil {
return config, err
} else {
config.RequiredFeatureSet = ptr.To[string](string(featureGate.Spec.FeatureSet))
}

if clusterVersion, err := client.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}); err != nil {
if clusterVersion, err := configv1client.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}); err != nil {
return config, err
} else {
config.Overrides = clusterVersion.Spec.Overrides
config.Capabilities = &clusterVersion.Status.Capabilities

// FIXME: eventually pull in GetImplicitlyEnabledCapabilities from https://github.com/openshift/cluster-version-operator/blob/86e24d66119a73f50282b66a8d6f2e3518aa0e15/pkg/payload/payload.go#L237-L240 for cases where a minor update would implicitly enable some additional capabilities. For now, 4.13 to 4.14 will always enable MachineAPI, ImageRegistry, etc..
currentVersion := clusterVersion.Status.Desired.Version
matches := regexp.MustCompile(`^(\d+[.]\d+)[.].*`).FindStringSubmatch(currentVersion)
if len(matches) < 2 {
return config, fmt.Errorf("failed to parse major.minor version from ClusterVersion status.desired.version %q", currentVersion)
} else if matches[1] == "4.13" {
build := configv1.ClusterVersionCapability("Build")
deploymentConfig := configv1.ClusterVersionCapability("DeploymentConfig")
imageRegistry := configv1.ClusterVersionCapability("ImageRegistry")
config.Capabilities.EnabledCapabilities = append(config.Capabilities.EnabledCapabilities, configv1.ClusterVersionCapabilityMachineAPI, build, deploymentConfig, imageRegistry)
config.Capabilities.KnownCapabilities = append(config.Capabilities.KnownCapabilities, configv1.ClusterVersionCapabilityMachineAPI, build, deploymentConfig, imageRegistry)
known := sets.New[configv1.ClusterVersionCapability](configv1.KnownClusterVersionCapabilities...)
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 •
edited
Loading

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?

config.Capabilities.KnownCapabilities = previouslyKnown.Union(known).UnsortedList()

// refresh BaselineCapabilitySet as more capabilities might be included across versions
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 •
edited
Loading

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.

}
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.

// The set of the capabilities may grow over time. Without downloading the payload that is running on the cluster,
// it is hard to project all the enabled capabilities after upgrading to the incoming release.
// As an approximation, all newly introduced capabilities are considered enabled to check if a manifest from the
// release should be included while some of them might not be actually enabled on the cluster.
// As a result, unexpected manifests could be included. The number of such manifests is likely small, provided that
// only a small amount of capabilities are added over time and that happens only for minor level updates:
// #Cap(4.11)=4 -> #Cap(4.17)=15, averagely less than two per minor update.
// https://docs.openshift.com/container-platform/4.17/installing/overview/cluster-capabilities.html
deltaKnown := known.Difference(previouslyKnown)
if deltaKnown.Len() > 0 {
klog.Infof("The new capabilities that are introduced in this oc version %s are considered enabled on checking if a manifest is included: %s. They may be disabled on the eventual cluster", ocVersion, deltaKnown.UnsortedList())
}
enabled = enabled.Union(deltaKnown)
config.Capabilities.EnabledCapabilities = sets.New[configv1.ClusterVersionCapability](config.Capabilities.EnabledCapabilities...).Union(enabled).UnsortedList()
}

if infrastructure, err := client.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}); err != nil {
if infrastructure, err := configv1client.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}); err != nil {
return config, err
} else if infrastructure.Status.PlatformStatus == nil {
return config, fmt.Errorf("cluster infrastructure does not declare status.platformStatus: %v", infrastructure.Status)
} else {
config.Platform = ptr.To[string](strings.ToLower(string(infrastructure.Status.PlatformStatus.Type)))
}

appsClient, err := appsv1client.NewForConfig(restConfig)
if err != nil {
return config, err
}

if deployment, err := appsClient.Deployments("openshift-cluster-version").Get(ctx, "cluster-version-operator", metav1.GetOptions{}); err != nil {
if deployment, err := appsv1client.Deployments("openshift-cluster-version").Get(ctx, "cluster-version-operator", metav1.GetOptions{}); err != nil {
return config, err
} else {
for _, container := range deployment.Spec.Template.Spec.Containers {
Expand Down
Loading