-
Notifications
You must be signed in to change notification settings - Fork 216
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
MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6868
base: master
Are you sure you want to change the base?
Conversation
@eliorerz: This pull request references MGMT-18561 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.18.0" version, but no target version was set. In 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. |
@eliorerz: GitHub didn't allow me to request PR reviews from the following users: eliorerz. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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 kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3599097
to
ffc0a0d
Compare
/test edge-subsystem-kubeapi-aws |
32d710e
to
404f4ed
Compare
/retest |
/retest |
1 similar comment
/retest |
@eliorerz: This pull request references MGMT-18561 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.18.0" version, but no target version was set. In 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. |
Tested the changes and verified it's trying the mirror registry 👍
|
/retest |
// MirrorRegistryConfigurationInfo holds the mirror registry configuration details | ||
type MirrorRegistryConfigurationInfo struct { | ||
ImageDigestMirrors []configv1.ImageDigestMirrors `json:"imageDigestMirrors,omitempty"` | ||
ImageTagMirrors []configv1.ImageTagMirrors `json:"imageTagMirrors,omitempty"` | ||
Insecure []string `json:"insecure,omitempty"` | ||
} | ||
|
||
// MirrorRegistryConfiguration holds the mirror registry configuration details | ||
type MirrorRegistryConfiguration struct { | ||
MirrorRegistryConfigurationInfo *MirrorRegistryConfigurationInfo `json:"mirrorRegistryConfigurationInfo,omitempty"` | ||
RegistriesConf string `json:"registriesConf,omitempty"` | ||
CaBundleCrt string `json:"caBundleCrt,omitempty"` | ||
} |
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.
These two types have the same description, yet they are different, can we distinguish between them in some meaningful way or combine them?
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.
One is used to holds the data and the other is to "only" hold the data that we want to display to the user in the status. I'll update the documentation.
api/v1beta1/infraenv_types.go
Outdated
// +optional | ||
OSImageVersion string `json:"osImageVersion,omitempty"` | ||
|
||
// MirrorRegistryRef is a reference to ClusterMirrorRegistry ConfigMap that holds the registries toml |
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 says "cluster" but an InfraEnv
shouldn't have cluster data. I think we should have a better description for this.
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.
That aws a mistake, I fofrgot to change the documentation when changed the property
// It will convert it to an ImageDigestMirrorSet CR and/or an ImageTagMirrorSet. | ||
// It will return these as marshalled JSON strings, and it will return a string list of insecure | ||
// mirror registries (if they exist) | ||
func GetImageRegistries(registryTOML string) ([]configv1.ImageDigestMirrors, []configv1.ImageTagMirrors, []string, 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.
Can we deduplicate and of this with ExtractLocationMirrorDataFromRegistriesFromToml
?
|
||
// MirrorRegistryConfigurationInfo contains the mirror registry configuration for this cluster | ||
// +optional | ||
MirrorRegistryConfigurationInfo *MirrorRegistryConfigurationInfo `json:"mirrorRegistryConfigurationInfo,omitempty"` |
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 are we setting this in status? Do you think it's very important to tell the user what they told us or are we using this in some way?
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.
Showing the user this information conveys two main points:
- Their TOML file is correctly configured and successfully fetching the passed information.
- When the user revisits this information, they can view the currently applied mirror registry configuration.
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.
Their TOML file is correctly configured and successfully fetching the passed information
Wouldn't we fail and set SpecSynced
condition with a helpful error of the file were malformed or we hit some other issue?
When the user revisits this information, they can view the currently applied mirror registry configuration.
🤷 I'm not sure it's more valuable to have multiple places where this is listed out (the original configmap and this status). If we say the spec is synced and they set the reference then that should be enough.
I would only expect something in status if it's being defined/decided by the controller rather than just repackaging information the user sent to us.
# Conflicts: # api/go.mod
…cluster mirror registry
…MirrorRegistryConfig
…rorRegistryConfig
404f4ed
to
110b923
Compare
/retest |
1 similar comment
/retest |
d9bfc7a
to
203d61f
Compare
@eliorerz: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In 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 kubernetes-sigs/prow repository. |
/test edge-subsystem-kubeapi-aws |
/retest |
Reverified the latest changes are still pulling from the mirror registry after the rebase
|
@@ -2325,3 +2346,27 @@ func getClusterDeploymentAdminKubeConfigSecretName(cd *hivev1.ClusterDeployment) | |||
} | |||
return fmt.Sprintf(adminKubeConfigStringTemplate, cd.Name) | |||
} | |||
|
|||
// processMirrorRegistryConfig retrieves the mirror registry configuration from the referenced ConfigMap | |||
func (r *ClusterDeploymentsReconciler) processMirrorRegistryConfig(ctx context.Context, log logrus.FieldLogger, clusterInstall *hiveext.AgentClusterInstall) (*hiveext.MirrorRegistryConfiguration, 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.
We should label this configmap with the backup label.
I think there's a function already for that.
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.
I'll add this logic (EnsureConfigMapIsLabelled
) at the end of ProcessMirrorRegistryConfig
that way it will ensure backup for both create and update
@@ -228,8 +229,13 @@ func (r *InfraEnvReconciler) updateInfraEnv(ctx context.Context, log logrus.Fiel | |||
updateParams.InfraEnvUpdateParams.KernelArguments = internalKernelArgs(infraEnv.Spec.KernelArguments) | |||
} | |||
|
|||
// UpdateInfraEnvInternal will generate an ISO only if there it was not generated before, | |||
return r.Installer.UpdateInfraEnvInternal(ctx, updateParams, nil) | |||
mirrorRegistryConfiguration, err := mirrorregistries.ProcessMirrorRegistryConfig(ctx, log, r.Client, infraEnv.Spec.MirrorRegistryRef) |
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.
Same in these cases, for both create and update we should ensure this configmap is backed up.
@@ -546,9 +547,14 @@ func (r *PreprovisioningImageReconciler) AddIronicAgentToInfraEnv(ctx context.Co | |||
return false, err | |||
} | |||
|
|||
mirrorRegistryConfiguration, err := mirrorregistries.ProcessMirrorRegistryConfig(ctx, log, r.Client, infraEnv.Spec.MirrorRegistryRef) |
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 function is only intended to set the internal override. We shouldn't need to also consider something the infraenv controller is already doing here. It's safe to remove this.
Did you consider adding the mirror registries config to the infraenv/cluster in the database? Why not go that way? |
Yes, this was my initial approach, but after discussing it with several people and identifying multiple edge cases (such as sync issues) without any real benefits, I decided not to implement it in the database. |
@eliorerz: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
@@ -5897,7 +5906,7 @@ func (b *bareMetalInventory) V2DownloadInfraEnvFiles(ctx context.Context, params | |||
switch params.FileName { | |||
case "discovery.ign": | |||
discoveryIsoType := swag.StringValue(params.DiscoveryIsoType) | |||
content, err = b.IgnitionBuilder.FormatDiscoveryIgnitionFile(ctx, infraEnv, b.IgnitionConfig, false, b.authHandler.AuthType(), discoveryIsoType) | |||
content, err = b.IgnitionBuilder.FormatDiscoveryIgnitionFile(ctx, infraEnv, b.IgnitionConfig, false, b.authHandler.AuthType(), discoveryIsoType, 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 means this override won't get into the discovery image. Is that okay?
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.
Feels like this really needs to be in the database for this to work.
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.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
/cc @eliorerz