-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MCO-2133: Select bootimages based on OSImageStream #10321
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewers: Please take a look at this change. It's the safest approach I could come up with. Basically, the structure of the ConfigMap is preserved and the stream field will continue to be reported, but from now on, it will use the default stream. A new streams field with a JSON map<streamName, streamJson> is now present. The format for SCOS remains unchanged, and the marketplace file is merged only if the file exists, which is only true for rhcos-9 for now. cc:@djoshy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,31 +9,51 @@ package main | |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "maps" | ||
| "os" | ||
| "path/filepath" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/coreos/stream-metadata-go/stream" | ||
| "github.com/coreos/stream-metadata-go/stream/rhcos" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "sigs.k8s.io/yaml" | ||
|
|
||
| instRhcos "github.com/openshift/installer/pkg/rhcos" | ||
| ) | ||
|
|
||
| const ( | ||
| streamRHCOSJSON = "data/data/coreos/rhcos.json" | ||
| streamSCOSJSON = "data/data/coreos/scos.json" | ||
| streamMarketplaceRHCOSJSON = "data/data/coreos/marketplace-rhcos.json" | ||
| scosTAG = "scos" | ||
| dest = "bin/manifests/coreos-bootimages.yaml" | ||
| scosTAG = "scos" | ||
| dest = "bin/manifests/coreos-bootimages.yaml" | ||
| ) | ||
|
|
||
| func run() error { | ||
| bootimages, err := getBootImages() | ||
| streamsFiles, err := getOSImageStreams() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var defaultStreamFiles *streamFiles | ||
| if len(streamsFiles) == 0 { | ||
| return fmt.Errorf("no OS image streams found") | ||
| } else if len(streamsFiles) == 1 { | ||
| defaultFile := streamsFiles[slices.Collect(maps.Keys(streamsFiles))[0]] | ||
| defaultStreamFiles = &defaultFile | ||
| } else { | ||
| rhel9Files, ok := streamsFiles[string(instRhcos.DefaultOSImageStream)] | ||
| if !ok { | ||
| return fmt.Errorf("no %v image streams found. %v is considered the default stream", instRhcos.DefaultOSImageStream, instRhcos.DefaultOSImageStream) | ||
| } | ||
| defaultStreamFiles = &rhel9Files | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use a more generic name like |
||
| } | ||
|
|
||
| defaultStreamBytes, err := defaultStreamFiles.getBytes() | ||
| if err != nil { | ||
| return fmt.Errorf("could not read default stream bytes: %w", err) | ||
| } | ||
|
|
||
| cm := &corev1.ConfigMap{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: corev1.SchemeGroupVersion.String(), | ||
|
|
@@ -50,10 +70,28 @@ func run() error { | |
| }, | ||
| Data: map[string]string{ | ||
| "releaseVersion": "0.0.1-snapshot", | ||
| "stream": string(bootimages), | ||
| "stream": string(defaultStreamBytes), | ||
| }, | ||
| } | ||
|
|
||
| if len(streamsFiles) > 1 { | ||
| data := &streamsData{ | ||
| Streams: make(map[string]string), | ||
| } | ||
| for name, files := range streamsFiles { | ||
| streamBytes, err := files.getBytes() | ||
| if err != nil { | ||
| return fmt.Errorf("could not read %s stream bytes: %w", name, err) | ||
| } | ||
| data.Streams[name] = string(streamBytes) | ||
| } | ||
| streamsJsonData, err := json.Marshal(data) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal streams data: %w", err) | ||
| } | ||
| cm.Data["streams"] = string(streamsJsonData) | ||
| } | ||
|
|
||
| b, err := yaml.Marshal(cm) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -71,64 +109,95 @@ func run() error { | |
| return nil | ||
| } | ||
|
|
||
| func getBootImages() ([]byte, error) { | ||
| var okd bool | ||
| var streamJSON string | ||
| tags, _ := os.LookupEnv("TAGS") | ||
| switch { | ||
| case strings.Contains(tags, scosTAG): | ||
| streamJSON = streamSCOSJSON | ||
| okd = true | ||
| default: | ||
| streamJSON = streamRHCOSJSON | ||
| } | ||
|
|
||
| bootimages, err := os.ReadFile(streamJSON) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if okd { | ||
| // okd does not yet have marketplace images, so we are done | ||
| return bootimages, nil | ||
| } | ||
|
|
||
| return mergeMarketplaceStream(bootimages) | ||
| type streamsData struct { | ||
| Streams map[string]string `json:"streams"` | ||
| } | ||
|
|
||
| type marketplaceStream map[string]*rhcos.Marketplace | ||
| type streamFiles struct { | ||
| streamFile string | ||
| marketplaceFile string | ||
| name string | ||
| } | ||
|
|
||
| func mergeMarketplaceStream(streamJSON []byte) ([]byte, error) { | ||
| mktStream := marketplaceStream{} | ||
| mktJSON, err := os.ReadFile(streamMarketplaceRHCOSJSON) | ||
| func (s *streamFiles) getBytes() ([]byte, error) { | ||
| streamJson, err := os.ReadFile(s.streamFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open marketplace file: %w", err) | ||
| } | ||
| if err := json.Unmarshal(mktJSON, &mktStream); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal market stream: %w", err) | ||
| return nil, fmt.Errorf("failed to open stream file: %w", err) | ||
| } | ||
|
|
||
| stream := stream.Stream{} | ||
| if err := json.Unmarshal(streamJSON, &stream); err != nil { | ||
| bootImageStream := stream.Stream{} | ||
| if err := json.Unmarshal(streamJson, &bootImageStream); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal boot image stream: %w", err) | ||
| } | ||
|
|
||
| for name, arch := range stream.Architectures { | ||
| if mkt, ok := mktStream[name]; ok { | ||
| if arch.RHELCoreOSExtensions == nil { | ||
| arch.RHELCoreOSExtensions = &rhcos.Extensions{} | ||
| if s.marketplaceFile != "" { | ||
| mktStream := marketplaceStream{} | ||
| mktJSON, err := os.ReadFile(s.marketplaceFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open marketplace file: %w", err) | ||
| } | ||
| if err := json.Unmarshal(mktJSON, &mktStream); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal market stream: %w", err) | ||
| } | ||
| for name, arch := range bootImageStream.Architectures { | ||
| if mkt, ok := mktStream[name]; ok { | ||
| if arch.RHELCoreOSExtensions == nil { | ||
| arch.RHELCoreOSExtensions = &rhcos.Extensions{} | ||
| } | ||
| arch.RHELCoreOSExtensions.Marketplace = mkt | ||
| } | ||
| arch.RHELCoreOSExtensions.Marketplace = mkt | ||
| } | ||
| } | ||
|
|
||
| bootImgs, err := json.Marshal(stream) | ||
| bootImgs, err := json.Marshal(bootImageStream) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal merged boot image stream: %w", err) | ||
| } | ||
| return bootImgs, nil | ||
| } | ||
|
|
||
| func getOSImageStreams() (map[string]streamFiles, error) { | ||
| name := "coreos" | ||
| if isOKD() { | ||
| name = "scos" | ||
| } | ||
| matches, err := filepath.Glob(fmt.Sprintf("data/data/coreos/%s*.json", name)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find image streams: %v", err) | ||
| } | ||
| streams := make(map[string]streamFiles) | ||
| for _, match := range matches { | ||
| filename := filepath.Base(match) | ||
| if strings.HasPrefix(filename, name+"-") { | ||
| // It's a stream | ||
|
|
||
| streamName := strings.TrimPrefix(strings.TrimSuffix(filename, ".json"), name+"-") | ||
| streamFile := streamFiles{ | ||
| streamFile: match, | ||
| name: streamName, | ||
| } | ||
|
|
||
| marketPlaceFile := filepath.Join(filepath.Dir(match), "marketplace-"+filename) | ||
| if _, err := os.Stat(marketPlaceFile); err == nil { | ||
| streamFile.marketplaceFile = marketPlaceFile | ||
| } | ||
|
Comment on lines
+173
to
+182
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also move marketplace files to a different directory... perhaps that would be cleaner. WDYT? |
||
| streams[streamName] = streamFile | ||
| } else if !strings.Contains(filename, "-") { | ||
| // It's a plain no OSImageStream, ie SCOS | ||
| streams[""] = streamFiles{ | ||
| streamFile: match, | ||
| } | ||
| } | ||
| } | ||
| return streams, nil | ||
| } | ||
|
|
||
| func isOKD() bool { | ||
| tags, _ := os.LookupEnv("TAGS") | ||
| return strings.Contains(tags, scosTAG) | ||
| } | ||
|
|
||
| type marketplaceStream map[string]*rhcos.Marketplace | ||
|
|
||
| func main() { | ||
| if err := run(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "%s\n", err) | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplest ever approach, should this be ok for now? I'm not too familiar with the concept of marketplace, so not really sure. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,10 +35,12 @@ type BaseIso struct { | |
| // CoreOSBuildFetcher will be to used to switch the source of the coreos metadata. | ||
| type CoreOSBuildFetcher func(ctx context.Context) (*stream.Stream, error) | ||
|
|
||
| func DefaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) { | ||
| return rhcos.FetchCoreOSBuild(ctx, rhcos.DefaultOSImageStream) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I'm missing something, but my expectations for this asset weres to modify
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't covered the agent cause I saw that I'd need to cover the case where the install-config is not provided but the AgentConfig is given, that means I need to add a field to it and so on. To avoid making the change bigger and bigger I thought about leaving the agent side consistent and ready for a follow up.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that as an incremental step. The first step usually is to support a new install-config field in the connected environment (ABI may be used also for that). With a simple change that part could be covered - otherwise with the current code a user may be confused by the fact that a config field was specified but "ignored".
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pablintino please disregard my previous comment. I understood (also from @djoshy comment) that for the 4.22 scope the current approach is correct (just default to rhcos.DefaultOSImageStream) |
||
| } | ||
|
|
||
| var ( | ||
| baseIsoFilename = "" | ||
| // DefaultCoreOSStreamGetter uses the pinned metadata. | ||
| DefaultCoreOSStreamGetter = rhcos.FetchCoreOSBuild | ||
| ) | ||
|
|
||
| var _ asset.WritableAsset = (*BaseIso)(nil) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fresh file @ravanelli provided, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file included in this PR simply for testing, or would we expect to merge it along with this PR?