-
Notifications
You must be signed in to change notification settings - Fork 39
AGENT-1193: Add --mirror-path flag to support pre-mirrored images #634
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: master
Are you sure you want to change the base?
Conversation
be9a81e to
c10ba39
Compare
|
@rwsu: This pull request references AGENT-1193 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
@rwsu: This pull request references AGENT-1193 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
@rwsu: This pull request references AGENT-1193 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
@rwsu: This pull request references AGENT-1193 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
/cc @danielerez |
cmd/build.go
Outdated
| // is reused throughout command execution. This prevents EnvConfig from | ||
| // being regenerated with default values when setupApplianceConfig() fetches | ||
| // ApplianceConfig, which would overwrite runtime flags like IsLiveISO. | ||
| assetStoreInstance asset.Store |
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 change probably worth a different PR, as it's not directly related to the new support, right?
Also, in which scenario it's required? I don't recall a specific flow that flags were overridden:/
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.
Yes, I retested and it is not needed. I will remove.
| # When provided, skips image mirroring and uses the pre-mirrored registry data. | ||
| # The path should point to an oc-mirror workspace directory containing a 'data' subdirectory. | ||
| # [Optional] | ||
| # mirrorPath: /path/to/mirror/workspace |
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.
probably worth adding also to the user-guide
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.
Done.
pkg/asset/config/appliance_config.go
Outdated
| } | ||
| releaseVersion = strings.Trim(releaseVersion, "'") | ||
| logrus.Debugf("Release version: %s", releaseVersion) | ||
| logrus.Debugf("Got release version: '%s'", releaseVersion) |
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.
not directly to change, and not sure this convention is cleaner:)
| } | ||
| releaseDigest = strings.Trim(releaseDigest, "'") | ||
| releaseImage = fmt.Sprintf("%s@%s", strings.Split(releaseImage, ":")[0], releaseDigest) | ||
| releaseImage = fmt.Sprintf("%s@%s", releaseImage, releaseDigest) |
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 change needed?
pkg/asset/config/appliance_config.go
Outdated
| } | ||
|
|
||
| // CleanupPullSecret removes the temporary pull secret file | ||
| func (a *ApplianceConfig) CleanupPullSecret() error { |
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.
where is it used?
| coreosImagePath := envConfig.FindInCache(coreosImagePattern) | ||
|
|
||
| // Add bootstrap scripts to ignition | ||
| logrus.Debugf("BootstrapIgnition rendering templates with IsLiveISO=%v", envConfig.IsLiveISO) |
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.
we already log this in env_config. isn't it enough?
| newYaml := strings.ReplaceAll(string(yamlBytes), buildRegistryURI, internalRegistryURI) | ||
|
|
||
| // Add IDMS entry for local registry mirror if using a custom release URL | ||
| if filepath.Base(yamlPath) == "idms-oc-mirror.yaml" { |
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.
isn't it generated by oc-mirror?
pkg/asset/config/appliance_config.go
Outdated
| configPath := filepath.Join(homeDir, ".docker", "config.json") | ||
| if err = os.MkdirAll(filepath.Dir(configPath), os.ModePerm); err != nil { | ||
| return err | ||
| // Write pull secret to temp file |
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.
why is it needed? we already store it in .docker/config.json, where oc can use it.
I mean, why not to keep lines 564-565?
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.
It is not needed. Will be removed.
Add support for using pre-mirrored images from oc-mirror workspace by
specifying the mirror path in appliance-config.yaml. When mirrorPath is
configured, the appliance skips running oc-mirror and uses the pre-mirrored
registry data directly.
Also fixes issues when using custom registries (non-quay.io) where oc adm
release info may return incomplete image references missing port and
repository path. Adds IDMS entries to ensure the cluster redirects pulls
from custom registries to the appliance's internal registry.
Changes:
- Add MirrorPath field to top level of ApplianceConfig in types
- Update appliance-config.yaml template to document the new field
- Update code to read mirrorPath from ApplianceConfig.Config.MirrorPath
- Add validateMirrorPath() to ApplianceConfig with comprehensive validation
- Use pre-mirrored images when mirrorPath is provided
- Add fixImageReference() to repair incomplete image refs from oc commands
Example: registry.example.com@sha256:... becomes
registry.example.com:5000/repo/image@sha256:...
- Add addLocalRegistryIDMS() to generate IDMS for custom registry mirrors
- Fix release image tag parsing to preserve port in registry URLs
- Update documentation to describe mirrorPath parameter and usage
The mirror-path can be specified in appliance-config.yaml as:
mirrorPath: /path/to/oc-mirror/workspace
Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rwsu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rwsu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Adds support for using pre-mirrored OCP release images instead of running
oc-mirror during the build process. This is useful when images have already
been mirrored in a separate step, avoiding redundant mirroring operations.
Changes:
When using custom registries instead of official OpenShift release images,
oc commands need authentication credentials to pull images. This adds the
--registry-config flag to all oc commands and --authfile to skopeo.
store instance and reusing it throughout the command execution, we ensure
EnvConfig is only generated once with the correct values and not overwritten.
Assisted-by: Claude Sonnet 4.5 noreply@anthropic.com