Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions pkg/asset/agent/image/agentimage.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func (a *AgentImage) Dependencies() []asset.Asset {
&manifests.AgentManifests{},
&BaseIso{},
&gencrypto.AuthConfig{},
&manifests.AgentOSImageStream{},
}
}

Expand All @@ -63,7 +64,8 @@ func (a *AgentImage) Generate(ctx context.Context, dependencies asset.Parents) e
agentArtifacts := &AgentArtifacts{}
agentManifests := &manifests.AgentManifests{}
baseIso := &BaseIso{}
dependencies.Get(agentArtifacts, agentManifests, baseIso, agentWorkflow, clusterInfo)
osImageStream := &manifests.AgentOSImageStream{}
dependencies.Get(agentArtifacts, agentManifests, baseIso, agentWorkflow, clusterInfo, osImageStream)

if err := workflowreport.GetReport(ctx).Stage(workflow.StageGenerateISO); err != nil {
return err
Expand Down Expand Up @@ -106,7 +108,7 @@ func (a *AgentImage) Generate(ctx context.Context, dependencies asset.Parents) e
logrus.Debugf("Using custom rootfs URL: %s", a.rootFSURL)
} else {
// Default to the URL from the RHCOS streams file
defaultRootFSURL, err := baseIso.getRootFSURL(ctx, a.cpuArch, agentWorkflow, clusterInfo)
defaultRootFSURL, err := baseIso.getRootFSURL(ctx, a.cpuArch, agentWorkflow, clusterInfo, osImageStream.Stream)
if err != nil {
return err
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/asset/agent/image/baseiso.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/sirupsen/logrus"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/agent"
"github.com/openshift/installer/pkg/asset/agent/joiner"
"github.com/openshift/installer/pkg/asset/agent/manifests"
"github.com/openshift/installer/pkg/asset/agent/mirror"
Expand All @@ -37,9 +36,8 @@ func (i *BaseIso) Name() string {
}

// Fetch RootFS URL using the rhcos.json.
func (i *BaseIso) getRootFSURL(ctx context.Context, archName string, agentWorkflow *workflow.AgentWorkflow, clusterInfo *joiner.ClusterInfo) (string, error) {
metal, err := rhcos.GetMetalArtifact(
ctx, archName, customStreamGetter(agentWorkflow, clusterInfo))
func (i *BaseIso) getRootFSURL(ctx context.Context, archName string, agentWorkflow *workflow.AgentWorkflow, clusterInfo *joiner.ClusterInfo, osImageStream types.OSImageStream) (string, error) {
metal, err := rhcos.GetMetalArtifact(ctx, archName, customStreamGetter(agentWorkflow, clusterInfo), osImageStream)
if err != nil {
return "", err
}
Expand All @@ -58,7 +56,7 @@ func (i *BaseIso) Dependencies() []asset.Asset {
&workflow.AgentWorkflow{},
&joiner.ClusterInfo{},
&manifests.AgentManifests{},
&agent.OptionalInstallConfig{},
&manifests.AgentOSImageStream{},
&mirror.RegistriesConf{},
}
}
Expand All @@ -69,11 +67,13 @@ func (i *BaseIso) Generate(ctx context.Context, dependencies asset.Parents) erro
registriesConf := &mirror.RegistriesConf{}
agentWorkflow := &workflow.AgentWorkflow{}
clusterInfo := &joiner.ClusterInfo{}
dependencies.Get(agentManifests, registriesConf, agentWorkflow, clusterInfo)
osImageStream := &manifests.AgentOSImageStream{}
dependencies.Get(agentManifests, registriesConf, agentWorkflow, clusterInfo, osImageStream)

baseIsoFileName, err := rhcos.NewBaseISOFetcher(
i.getRelease(agentManifests, registriesConf.MirrorConfig),
customStreamGetter(agentWorkflow, clusterInfo)).GetBaseISOFilename(ctx, agentManifests.InfraEnv.Spec.CpuArchitecture)
customStreamGetter(agentWorkflow, clusterInfo),
osImageStream.Stream).GetBaseISOFilename(ctx, agentManifests.InfraEnv.Spec.CpuArchitecture)

if err == nil {
logrus.Debugf("Using base ISO image %s", baseIsoFileName)
Expand Down
10 changes: 6 additions & 4 deletions pkg/asset/agent/image/ignition.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func (a *Ignition) Dependencies() []asset.Asset {
&mirror.CaBundle{},
&gencrypto.AuthConfig{},
&common.InfraEnvID{},
&manifests.AgentOSImageStream{},
}
}

Expand All @@ -128,7 +129,8 @@ func (a *Ignition) Generate(ctx context.Context, dependencies asset.Parents) err
fencingCredentials := &agentconfig.FencingCredentials{}
authConfig := &gencrypto.AuthConfig{}
infraEnvAsset := &common.InfraEnvID{}
dependencies.Get(agentManifests, agentConfigAsset, agentHostsAsset, extraManifests, fencingCredentials, authConfig, agentWorkflow, infraEnvAsset)
osImageStream := &manifests.AgentOSImageStream{}
dependencies.Get(agentManifests, agentConfigAsset, agentHostsAsset, extraManifests, fencingCredentials, authConfig, agentWorkflow, infraEnvAsset, osImageStream)
clusterInfo := &joiner.ClusterInfo{}

if err := workflowreport.GetReport(ctx).Stage(workflow.StageIgnition); err != nil {
Expand Down Expand Up @@ -269,7 +271,7 @@ func (a *Ignition) Generate(ctx context.Context, dependencies asset.Parents) err
infraEnvID := infraEnvAsset.ID
logrus.Debug("Generated random infra-env id ", infraEnvID)

osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, customStreamGetter(agentWorkflow, clusterInfo))
osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, customStreamGetter(agentWorkflow, clusterInfo), osImageStream.Stream)
if err != nil {
return err
}
Expand Down Expand Up @@ -745,13 +747,13 @@ func addExtraManifests(config *igntypes.Config, extraManifests *manifests.ExtraM
return nil
}

func getOSImagesInfo(ctx context.Context, cpuArch string, openshiftVersion string, streamGetter rhcos.CoreOSBuildFetcher) (*models.OsImage, error) {
func getOSImagesInfo(ctx context.Context, cpuArch string, openshiftVersion string, streamGetter rhcos.CoreOSBuildFetcher, osImageStream types.OSImageStream) (*models.OsImage, error) {
osImage := &models.OsImage{
CPUArchitecture: &cpuArch,
}
osImage.OpenshiftVersion = &openshiftVersion

artifacts, err := rhcos.GetMetalArtifact(ctx, cpuArch, streamGetter)
artifacts, err := rhcos.GetMetalArtifact(ctx, cpuArch, streamGetter, osImageStream)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/agent/image/unconfigured_ignition.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (a *UnconfiguredIgnition) Generate(ctx context.Context, dependencies asset.
if err != nil {
return err
}
osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, nil)
osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, nil, "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: for readability, rather than an empty string a more speaking constant could be better

if err != nil {
return err
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/asset/agent/manifests/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/openshift/assisted-service/models"
hivev1 "github.com/openshift/hive/apis/hive/v1"
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/manifests"
)

func TestAgentManifests_Generate(t *testing.T) {
Expand Down Expand Up @@ -59,6 +60,7 @@ func TestAgentManifests_Generate(t *testing.T) {
&AgentClusterInstall{Config: fakeAgentClusterInstall},
&ClusterDeployment{Config: fakeClusterDeployment},
&ClusterImageSet{Config: fakeClusterImageSet},
&manifests.OSImageStream{},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned before, very likely this will not be required

},
ExpectedPullSecret: fakeSecret,
ExpectedInfraEnv: fakeInfraEnv,
Expand Down Expand Up @@ -93,6 +95,7 @@ func TestAgentManifests_Generate(t *testing.T) {
&AgentClusterInstall{},
&ClusterDeployment{},
&ClusterImageSet{},
&manifests.OSImageStream{},
},
ExpectedError: "invalid agent configuration: spec.nmStateConfigLabelSelector.matchLabels: Required value: infraEnv and fake-nmState NMState Config labels do not match. Expected: map[missing-label:missing-label] Found: map[]",
},
Expand Down
7 changes: 7 additions & 0 deletions pkg/asset/agent/manifests/agentclusterinstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ type agentClusterInstallInstallConfigOverrides struct {
FeatureSet configv1.FeatureSet `json:"featureSet,omitempty"`
// Allow override of FeatureGates
FeatureGates []string `json:"featureGates,omitempty"`
// OSImageStream is the OS Image Stream to be used for all machines in the cluster
OSImageStream types.OSImageStream `json:"osImageStream,omitempty"`
}

var _ asset.WritableAsset = (*AgentClusterInstall)(nil)
Expand Down Expand Up @@ -262,6 +264,11 @@ func (a *AgentClusterInstall) Generate(_ context.Context, dependencies asset.Par
icOverrides.FeatureGates = installConfig.Config.FeatureGates
}

if installConfig.Config.OSImageStream != "" {
icOverridden = true
icOverrides.OSImageStream = installConfig.Config.OSImageStream
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

}

if installConfig.Config.Proxy != nil {
rendezvousIP := ""
if agentConfig.Config != nil {
Expand Down
118 changes: 118 additions & 0 deletions pkg/asset/agent/manifests/osimagestream.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name of this asset file should be agentosimagestream.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It'd be nice also to have a dedicated test file for this new asset

Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package manifests

import (
"context"
"encoding/json"

"github.com/sirupsen/logrus"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

hiveext "github.com/openshift/assisted-service/api/hiveextension/v1beta1"
mcfgclient "github.com/openshift/client-go/machineconfiguration/clientset/versioned"
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/agent/joiner"
"github.com/openshift/installer/pkg/asset/agent/workflow"
"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/types"
)

// installConfigOverridesData is used to parse the installConfigOverrides annotation.
type installConfigOverridesData struct {
OSImageStream types.OSImageStream `json:"osImageStream,omitempty"`
}

// AgentOSImageStream exposes the OS image stream value for Agent workflows.
// This is a pure data asset that does not generate any files.
type AgentOSImageStream struct {
Stream types.OSImageStream
}

var _ asset.Asset = (*AgentOSImageStream)(nil)

// Name returns the human-friendly name of the asset.
func (*AgentOSImageStream) Name() string {
return "Agent OS Image Stream"
}

// Dependencies returns all of the dependencies directly needed to generate
// the asset.
func (*AgentOSImageStream) Dependencies() []asset.Asset {
return []asset.Asset{
&workflow.AgentWorkflow{},
&joiner.ClusterInfo{},
&AgentManifests{},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works, but since the current asset requires only AgentClusterInstall it should be sufficient to have, as a dependency, just &manifests.AgentClusterInstall{} (useful to minimize the dependency requirements)

}
}

// Generate extracts the OS image stream value from available sources.
func (a *AgentOSImageStream) Generate(ctx context.Context, dependencies asset.Parents) error {
agentWorkflow := &workflow.AgentWorkflow{}
clusterInfo := &joiner.ClusterInfo{}
agentManifests := &AgentManifests{}
dependencies.Get(agentWorkflow, clusterInfo, agentManifests)

var stream types.OSImageStream

switch agentWorkflow.Workflow {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a slightly unusual pattern, since usually this kind of logic (discriminating between the Install and AddNodes workflow) is applied directly into the assets that need to consume the dependencies values.
In this case the asset does not consume the field but hides the implementation details about osImageStream retrieval, and exposes directly the value as a field meant to be consumed by other assets.
The good point is that, since this field is required by many different consumer, the retrieval logic is not duplicated across them, but remains centralized here (so fewer risks of different usages in different places). The drawback is that such logic remains a little bit less visible in the asset consumer code (as it keeps having its own workflow discriminating logic).
Even though I'm a little bit more inclined to improved readability, I'm not opposed to such approach

case workflow.AgentWorkflowTypeInstall:
// For install workflow, read from AgentClusterInstall manifest (Layer 2)
if agentManifests.AgentClusterInstall != nil {
stream = a.getStreamFromAgentClusterInstall(agentManifests.AgentClusterInstall)
}

case workflow.AgentWorkflowTypeAddNodes:
// For add-nodes workflow, query the cluster's OSImageStream CR
if clusterInfo.OpenshiftMachineConfigClient != nil {
var err error
stream, err = a.getStreamFromCluster(ctx, clusterInfo.OpenshiftMachineConfigClient)
if err != nil {
// Log but don't fail - fall back to default
logrus.Warnf("Failed to query cluster OSImageStream: %v", err)
}
}
}

// Default
if stream == "" {
stream = rhcos.DefaultOSImageStream
}

a.Stream = stream
return nil
}

// getStreamFromAgentClusterInstall extracts the osImageStream from the
// AgentClusterInstall installConfigOverrides annotation.
func (a *AgentOSImageStream) getStreamFromAgentClusterInstall(aci *hiveext.AgentClusterInstall) types.OSImageStream {
if aci.Annotations == nil {
return ""
}

overridesJSON, ok := aci.Annotations[installConfigOverrides]
if !ok {
return ""
}

var overrides installConfigOverridesData
if err := json.Unmarshal([]byte(overridesJSON), &overrides); err != nil {
logrus.Debugf("Failed to parse installConfigOverrides: %v", err)
return ""
}

return overrides.OSImageStream
}

// getStreamFromCluster queries the cluster's OSImageStream CR to determine
// the default stream.
func (a *AgentOSImageStream) getStreamFromCluster(ctx context.Context, client mcfgclient.Interface) (types.OSImageStream, error) {
osImageStream, err := client.MachineconfigurationV1alpha1().OSImageStreams().Get(ctx, "cluster", metav1.GetOptions{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not look correct, since it's a ClusterInfo responsibility to retrieve the osImageStream. The current asset should just fetch this information from the ClusterInfo dependency (through a field), and leave to ClusterInfo the decision on how to retrieve it. So the extraction logic should be refactored inside ClusterInfo

if err != nil {
return "", err
}

if osImageStream.Spec == nil {
return "", nil
}

return types.OSImageStream(osImageStream.Spec.DefaultStream), nil
}
36 changes: 28 additions & 8 deletions pkg/asset/manifests/osimagestream.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given the new specific agen asset AgenOSImageStream, all the changes here don't seem to still needed, isn't it?

Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ import (
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/types"
)

var osImageStreamFileName = path.Join(openshiftManifestDir, "99_osimagestream.yaml")

// OSImageStream generates the OSImageStream manifest.
// OSImageStream generates the OSImageStream manifest and exposes the stream value.
type OSImageStream struct {
FileList []*asset.File
Stream types.OSImageStream
}

var _ asset.WritableAsset = (*OSImageStream)(nil)
Expand All @@ -42,20 +44,39 @@ func (f *OSImageStream) Generate(_ context.Context, dependencies asset.Parents)
installConfig := &installconfig.InstallConfig{}
dependencies.Get(installConfig)

if installConfig.Config == nil {
return nil
}

config := installConfig.Config

// If one of the following are true the OSImageStream CR is not generated
// 1. The feature is not enabled
// 2. The target is CentOS Stream CoreOS
if ic := installConfig.Config; !ic.Enabled(features.FeatureGateOSStreams) || ic.IsSCOS() {
if !config.Enabled(features.FeatureGateOSStreams) || config.IsSCOS() {
// FG disabled or not OCP
return nil
}

// If no stream was set just report the default one for the current version
stream := installConfig.Config.OSImageStream
// Extract and store the stream value
stream := config.OSImageStream
if stream == "" {
stream = rhcos.DefaultOSImageStream
}
f.Stream = stream

// Generate manifest file
manifest, err := f.createManifest(stream)
if err != nil {
return err
}
f.FileList = append(f.FileList, manifest)

return nil
}

// createManifest generates the OSImageStream manifest file.
func (f *OSImageStream) createManifest(stream types.OSImageStream) (*asset.File, error) {
osImageStream := &mcfgv1alpha.OSImageStream{
TypeMeta: metav1.TypeMeta{
APIVersion: mcfgv1alpha.SchemeGroupVersion.String(),
Expand All @@ -71,14 +92,13 @@ func (f *OSImageStream) Generate(_ context.Context, dependencies asset.Parents)

osImageStreamData, err := yaml.Marshal(osImageStream)
if err != nil {
return fmt.Errorf("failed to create %s manifests from InstallConfig. Error: %w", f.Name(), err)
return nil, fmt.Errorf("failed to create %s manifests from InstallConfig. Error: %w", f.Name(), err)
}

f.FileList = append(f.FileList, &asset.File{
return &asset.File{
Filename: osImageStreamFileName,
Data: osImageStreamData,
})
return nil
}, nil
}

// Files returns the files generated by the asset.
Expand Down
Loading