Skip to content

NO-JIRA: Include the target version in the logging #2050

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

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

NO-JIRA: Include the target version in the logging #2050

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
26 changes: 24 additions & 2 deletions pkg/cli/admin/release/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,28 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
}
}

// versionInImageConfig stores the version from the label of the image
// At the moment it is used later only for the logging purpose and thus is not blocking if failures occur upon getting its value
Copy link
Member

Choose a reason for hiding this comment

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

If the errors are not blocking then maybe they should be logged as warnings, not errors?

var versionInImageConfig string
opts.ImageConfigCallback = func(imageConfig *dockerv1client.DockerImageConfig) {
if imageConfig == nil {
// This should never happen
klog.Error("Cannot retrieve the version because no image configuration is provided in the image to extract")
return
}
if imageConfig.Config == nil {
klog.Error("Cannot retrieve the version from image configuration in the image to extract because it has no configuration")
return
}
v, ok := imageConfig.Config.Labels["io.openshift.release"]
if !ok {
klog.Error("Cannot retrieve the version from image configuration in the image to extract because it does not have the required label 'io.openshift.release'")
return
}
klog.V(2).Infof("Retrieved the version from image configuration in the image to extract: %s", v)
versionInImageConfig = v
Copy link
Member

Choose a reason for hiding this comment

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

We assume we never reach this place twice here (= we assume the callback is never called multiple time in a way that reaches this assignment) otherwise the old value is overwritten.

That assumption may be fine but I'd either add a comment here about it or logged if we reach here while versionInImageConfig is not empty.

}

var manifestErrs []error
// o.ExtractManifests implies o.File == ""
if o.ExtractManifests {
Expand All @@ -359,9 +381,9 @@ func (o *ExtractOptions) Run(ctx context.Context) error {
context := "connected cluster"
inclusionConfig := manifestInclusionConfiguration{}
if o.InstallConfig == "" {
inclusionConfig, err = findClusterIncludeConfig(ctx, o.RESTConfig)
inclusionConfig, err = findClusterIncludeConfig(ctx, o.RESTConfig, versionInImageConfig)
} else {
inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig)
inclusionConfig, err = findClusterIncludeConfigFromInstallConfig(ctx, o.InstallConfig, versionInImageConfig)
Comment on lines +384 to +386
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something but I don't understand when is versionInImageConfig actually populated. What I'm reading is:

  1. It is empty on L354 when declared
  2. It can be populated on L371 inside the opts.ImageConfigCallback callback (which itself is populated on L355)
  3. It can only be actually populated once opts.ImageConfigCallback is called at least one
  4. The callback is called in pkg/cli/image/extract/extract.go:L426 in o.Run()
  5. Here on L384 / L386 we expect this to be a populated string already but I do not see any opportunity betwen L372 and the L383 if where o.Run() could be called, even indirectly.

How is versionInImageConfig not always an empty string on L384/L386??

Copy link
Member

Choose a reason for hiding this comment

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

This is reiterated #1958 (comment) but I do not understand the answer.

context = o.InstallConfig
}
if err != nil {
Expand Down
33 changes: 24 additions & 9 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 @@ -1167,16 +1167,29 @@ func copyAndReplace(errorOutput io.Writer, w io.Writer, r io.Reader, bufferSize

}

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

// logCapabilitySetMayDiffer logs the messages if the oc-cli version is not the same as the target version
// * the enabled capability set may differ if the baseline is vCurrent, and
// * the known capability set may differ
// It raises an error if it fails to determine the oc-cli version.
func logCapabilitySetMayDiffer(capabilitiesSpec *configv1.ClusterVersionCapabilitiesSpec, targetVersion string) error {
clientVersion, reportedVersion, err := version.ExtractVersion()
if err != nil {
return config, err
return fmt.Errorf("failed to determine the version of 'oc': %w", err)
}
if reportedVersion == "" {
reportedVersion = clientVersion.String()
}
if reportedVersion != targetVersion {
if capabilitiesSpec != nil && capabilitiesSpec.BaselineCapabilitySet == configv1.ClusterVersionCapabilitySetCurrent {
klog.Infof("The eventual cluster %s will not be the same minor version as this %s 'oc', the actual %s capability set may differ.", targetVersion, reportedVersion, capabilitiesSpec.BaselineCapabilitySet)
}
klog.Infof("The eventual cluster %s will not be the same minor version as this %s 'oc', the known capability set may differ.", targetVersion, reportedVersion)
}
return nil
}

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

installConfigBytes, err := os.ReadFile(installConfigPath)
if err != nil {
Expand Down Expand Up @@ -1204,21 +1217,19 @@ func findClusterIncludeConfigFromInstallConfig(ctx context.Context, installConfi
if enabled, ok := configv1.ClusterVersionCapabilitySets[data.Capabilities.BaselineCapabilitySet]; !ok {
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)
if err := logCapabilitySetMayDiffer(data.Capabilities, versionInImageConfig); err != nil {
return config, err
}
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)
config.Capabilities.KnownCapabilities = configv1.KnownClusterVersionCapabilities
}

return config, nil
}

func findClusterIncludeConfig(ctx context.Context, restConfig *rest.Config) (manifestInclusionConfiguration, error) {
func findClusterIncludeConfig(ctx context.Context, restConfig *rest.Config, versionInImageConfig string) (manifestInclusionConfiguration, error) {
config := manifestInclusionConfiguration{}

client, err := configv1client.NewForConfig(restConfig)
Expand Down Expand Up @@ -1250,6 +1261,10 @@ func findClusterIncludeConfig(ctx context.Context, restConfig *rest.Config) (man
config.Capabilities.EnabledCapabilities = append(config.Capabilities.EnabledCapabilities, configv1.ClusterVersionCapabilityMachineAPI, build, deploymentConfig, imageRegistry)
config.Capabilities.KnownCapabilities = append(config.Capabilities.KnownCapabilities, configv1.ClusterVersionCapabilityMachineAPI, build, deploymentConfig, imageRegistry)
}

if err := logCapabilitySetMayDiffer(clusterVersion.Spec.Capabilities, versionInImageConfig); err != nil {
return config, err
}
}

if infrastructure, err := client.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions pkg/cli/image/extract/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ type ExtractOptions struct {

genericiooptions.IOStreams

// ImageConfigCallback is invoked once image config retrieved
ImageConfigCallback func(imageConfig *dockerv1client.DockerImageConfig)
// ImageMetadataCallback is invoked once per image retrieved, and may be called in parallel if
// MaxPerRegistry is set higher than 1.
ImageMetadataCallback ImageMetadataFunc
Expand Down Expand Up @@ -421,6 +423,9 @@ func (o *ExtractOptions) Run() error {
if err != nil {
return fmt.Errorf("unable to parse image %s: %v", from, err)
}
if o.ImageConfigCallback != nil {
o.ImageConfigCallback(imageConfig)
}

if mapping.ConditionFn != nil {
ok, err := mapping.ConditionFn(&mapping, location.Manifest, imageConfig)
Expand Down