MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6965

eliorerz · 2024-11-03T09:21:54Z

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

/cc @eliorerz

openshift-ci-robot · 2024-11-03T09:21:57Z

openshift-ci · 2024-11-03T09:22:01Z

openshift-ci · 2024-11-03T09:22:28Z

[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:

~~OWNERS~~ [eliorerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2024-11-03T09:57:34Z

Codecov Report

Attention: Patch coverage is 58.26446% with 101 lines in your changes missing coverage. Please review.

Project coverage is 68.73%. Comparing base (7a08e50) to head (015f338).

Files with missing lines	Patch %	Lines
internal/common/db.go	0.00%	39 Missing ⚠️
internal/installcfg/builder/builder.go	62.71%	15 Missing and 7 partials ⚠️
internal/bminventory/inventory.go	57.14%	7 Missing and 5 partials ⚠️
internal/ignition/discovery.go	62.96%	6 Missing and 4 partials ⚠️
...rnal/controller/controllers/infraenv_controller.go	52.94%	4 Missing and 4 partials ⚠️
pkg/mirrorregistries/parser.go	82.92%	5 Missing and 2 partials ⚠️
...oller/controllers/clusterdeployments_controller.go	90.90%	1 Missing and 1 partial ⚠️
pkg/mirrorregistries/generator.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6965      +/-   ##
==========================================
- Coverage   68.80%   68.73%   -0.08%     
==========================================
  Files         251      252       +1     
  Lines       37635    37795     +160     
==========================================
+ Hits        25896    25977      +81     
- Misses       9420     9485      +65     
- Partials     2319     2333      +14

Files with missing lines	Coverage Δ
internal/bminventory/inventory_v2_handlers.go	`56.60% <100.00%> (ø)`
...ler/controllers/preprovisioningimage_controller.go	`81.81% <100.00%> (ø)`
internal/oc/release.go	`73.78% <100.00%> (ø)`
pkg/mirrorregistries/generator.go	`67.64% <83.33%> (+3.06%)`	⬆️
...oller/controllers/clusterdeployments_controller.go	`71.68% <90.90%> (+0.15%)`	⬆️
pkg/mirrorregistries/parser.go	`82.92% <82.92%> (ø)`
...rnal/controller/controllers/infraenv_controller.go	`63.44% <52.94%> (-0.46%)`	⬇️
internal/ignition/discovery.go	`69.51% <62.96%> (+0.22%)`	⬆️
internal/bminventory/inventory.go	`70.53% <57.14%> (-0.18%)`	⬇️
internal/installcfg/builder/builder.go	`79.59% <62.71%> (-3.34%)`	⬇️
... and 1 more

... and 1 file with indirect coverage changes

eliorerz · 2024-11-03T11:23:22Z

/retest

eliorerz · 2024-11-03T17:53:31Z

/cc @carbonin @CrystalChun
I moved the previous PR to here and added the mirror registry to the database

CrystalChun · 2024-11-05T17:42:41Z

Retested and verified it's pulling from the mirror registry 👍

  Warning  Failed          1s (x2 over 15s)   kubelet            Failed to pull image "quay.io/cchun/testing@sha256:e1f8ac471c4cd63333590309829aa8f1210a62571179d39fc6989ab6a9c09e2e": (Mirrors also failed: [10.1.178.25:5000/cchun/testing@sha256:e1f8ac471c4cd63333590309829aa8f1210a62571179d39fc6989ab6a9c09e2e: pinging container registry 10.1.178.25:5000: Get "https://10.1.178.25:5000/v2/": http: server gave HTTP response to HTTPS client]): quay.io/cchun/testing@sha256:e1f8ac471c4cd63333590309829aa8f1210a62571179d39fc6989ab6a9c09e2e: reading manifest sha256:e1f8ac471c4cd63333590309829aa8f1210a62571179d39fc6989ab6a9c09e2e in quay.io/cchun/testing: unauthorized: access to the requested resource is not authorized

internal/bminventory/inventory.go

internal/bminventory/inventory_test.go

internal/bminventory/inventory.go

internal/ignition/discovery.go

internal/ignition/discovery_test.go

internal/installcfg/builder/builder.go

pkg/mirrorregistries/mirror_registry.go

eliorerz · 2024-11-08T13:37:42Z

/retest

eliorerz · 2024-11-10T08:02:42Z

/retest

…scovery step

… and cluster DB

…cluster mirror registry # Conflicts: # internal/controller/controllers/clusterdeployments_controller_test.go

…MirrorRegistryConfig

…rorRegistryConfig

openshift-ci · 2024-11-12T21:42:11Z

@eliorerz: all tests passed!

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.

carbonin · 2024-11-14T14:32:49Z

api/common/common_types.go

@@ -16,3 +18,12 @@ type ValidationsStatus map[string]ValidationResults

 // +kubebuilder:object:generate=true
 type ValidationResults []ValidationResult
+
+// MirrorRegistryConfiguration holds the given mirror registry configuration
+type MirrorRegistryConfiguration struct {


I still think this doesn't belong in the api module.

It's not a user-facing API. Let's put it in internal/common/db.go with the functions that manage it's representation in the db.

carbonin · 2024-11-14T15:08:37Z

internal/controller/controllers/mirror_registry/mirror_registry_parser.go

@@ -0,0 +1,102 @@
+package mirror_registry


I think generally the convention is to avoid underscores in package names. I think this one would be fine without it.

carbonin · 2024-11-14T15:13:58Z

internal/controller/controllers/preprovisioningimage_controller.go

@@ -548,7 +548,7 @@ func (r *PreprovisioningImageReconciler) AddIronicAgentToInfraEnv(ctx context.Co

 	updated := false
 	if string(conf) != infraEnvInternal.InternalIgnitionConfigOverride {
-		_, err = r.Installer.UpdateInfraEnvInternal(ctx, installer.UpdateInfraEnvParams{InfraEnvID: *infraEnvInternal.ID, InfraEnvUpdateParams: &models.InfraEnvUpdateParams{}}, swag.String(string(conf)))
+		_, err = r.Installer.UpdateInfraEnvInternal(ctx, installer.UpdateInfraEnvParams{InfraEnvID: *infraEnvInternal.ID, InfraEnvUpdateParams: &models.InfraEnvUpdateParams{}}, swag.String(string(conf)), nil)


Won't this unset the mirror registries?

carbonin · 2024-11-14T15:15:06Z

internal/ignition/discovery.go

 	clusterPkg "github.com/openshift/assisted-service/internal/cluster"
 	"github.com/openshift/assisted-service/internal/common"
 	"github.com/openshift/assisted-service/internal/constants"
+	"github.com/openshift/assisted-service/internal/controller/controllers/mirror_registry"


Can we use an alias for these to differentiate between pkg/mirrorregistries and controllers/mirror_registry (or whatever it ends up being called)

carbonin · 2024-11-14T15:20:02Z

internal/ignition/discovery.go

+	mirrorRegistriesConfigKey := "MirrorRegistriesConfig"
+	mirrorRegistriesCAConfigKey := "MirrorRegistriesCAConfig"


Should these be constants?

carbonin · 2024-11-14T15:20:39Z

internal/ignition/discovery.go

+		(ignitionParams)[mirrorRegistriesConfigKey] = base64.StdEncoding.EncodeToString([]byte(configuration.RegistriesConf))
+		(ignitionParams)[mirrorRegistriesCAConfigKey] = base64.StdEncoding.EncodeToString([]byte(configuration.CaBundleCrt))


Why parens here?

carbonin · 2024-11-14T15:21:39Z

internal/ignition/discovery.go

+	mirrorRegistriesConfigKey := "MirrorRegistriesConfig"
+	mirrorRegistriesCAConfigKey := "MirrorRegistriesCAConfig"
+
+	if mirror_registry.IsMirrorConfigurationSet(configuration) {


Maybe a comment here that this is the config from the infraenv, vs the following one which is defined in the environment.

carbonin · 2024-11-14T15:21:57Z

internal/ignition/discovery.go

+		(ignitionParams)[mirrorRegistriesConfigKey] = base64.StdEncoding.EncodeToString(registriesContents)
+		(ignitionParams)[mirrorRegistriesCAConfigKey] = base64.StdEncoding.EncodeToString(caContents)


Same here, not sure why this needs parens

carbonin · 2024-11-14T15:23:16Z

internal/installcfg/builder/builder.go

+func (i *installConfigBuilder) setImageDigestMirrorSet(cfg *installcfg.InstallerConfigBaremetal, configuration *common_api.MirrorRegistryConfiguration) error {
+	var imageDigestSourceList []installcfg.ImageDigestSource
+
+	if mirror_registry.IsMirrorConfigurationSet(configuration) {


Same in here, can we have a comment in these functions saying which block is the cluster config and which is the environmental one?

carbonin · 2024-11-14T15:48:44Z

pkg/mirrorregistries/generator.go

-		}
-		registriesConfList[i] = RegistriesConf{Location: location, Mirror: mirrors}
+	for _, i := range idmsMirrors {
+		mirrors := *(*[]string)(unsafe.Pointer(&i.Mirrors))


What is going on here? Feels like this shouldn't be necessary. If it is please add a comment of some sort.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 3, 2024

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. api-review Categorizes an issue or PR as actively needing an API review. labels Nov 3, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2024

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from 67b98f8 to fd90bbd Compare November 3, 2024 12:54

openshift-ci bot requested review from carbonin and CrystalChun November 3, 2024 17:53

eliorerz mentioned this pull request Nov 5, 2024

MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6868

Closed

16 tasks

carbonin reviewed Nov 5, 2024

View reviewed changes

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from a1af42c to 8ec7bc2 Compare November 7, 2024 13:54

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from 8ec7bc2 to cc43d9f Compare November 11, 2024 09:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from cc43d9f to eddba1a Compare November 12, 2024 17:38

eliorerz added 7 commits November 12, 2024 19:38

MGMT-18561: Add AgentClusterInstall mirror registry generated files

1e74ece

MGMT-18562: Add InfraEnv mirror registry generated files for hosts di…

b085155

…scovery step

MGMT-18561: MGMT-18562: Add mirror registry configuration to infraenv…

1bab105

… and cluster DB

MGMT-18561: MGMT-18562: Update mirrorregistry package to support per …

14a6aba

…cluster mirror registry # Conflicts: # internal/controller/controllers/clusterdeployments_controller_test.go

MGMT-18561: Update install-config ImageDigestSource by the generated …

1dd6b6b

…MirrorRegistryConfig

MGMT-18562: Update ignition registries.conf file by the generated Mir…

9891450

…rorRegistryConfig

MGMT-18561: MGMT-18562: Add subsystem tests

2b31f57

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from eddba1a to 98c1f6e Compare November 12, 2024 17:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from 98c1f6e to b97b322 Compare November 12, 2024 17:49

MGMT-18561: MGMT-18562:

015f338

eliorerz force-pushed the MGMT-18561-MGMT-18562-Add-KubeAPI-support-for-adding-mirror-registry-in-AgentClusterInstall-db branch from b97b322 to 015f338 Compare November 12, 2024 18:13

carbonin reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6965

MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6965

eliorerz commented Nov 3, 2024

openshift-ci-robot commented Nov 3, 2024 •

edited by openshift-ci bot

Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

openshift-ci bot commented Nov 3, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

openshift-ci bot commented Nov 3, 2024

codecov bot commented Nov 3, 2024 •

edited

Loading

eliorerz commented Nov 3, 2024

eliorerz commented Nov 3, 2024 •

edited

Loading

CrystalChun commented Nov 5, 2024

eliorerz commented Nov 8, 2024

eliorerz commented Nov 10, 2024

openshift-ci bot commented Nov 12, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

carbonin Nov 14, 2024

		mirrorRegistriesConfigKey := "MirrorRegistriesConfig"
		mirrorRegistriesCAConfigKey := "MirrorRegistriesCAConfig"

		(ignitionParams)[mirrorRegistriesConfigKey] = base64.StdEncoding.EncodeToString([]byte(configuration.RegistriesConf))
		(ignitionParams)[mirrorRegistriesCAConfigKey] = base64.StdEncoding.EncodeToString([]byte(configuration.CaBundleCrt))

MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6965

Are you sure you want to change the base?

MGMT-18561: MGMT-18562: Add kube api support for adding per cluster mirror registry in AgentClusterInstall #6965

Conversation

eliorerz commented Nov 3, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

openshift-ci-robot commented Nov 3, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

openshift-ci bot commented Nov 3, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

openshift-ci bot commented Nov 3, 2024

codecov bot commented Nov 3, 2024 • edited Loading

Codecov Report

eliorerz commented Nov 3, 2024

eliorerz commented Nov 3, 2024 • edited Loading

CrystalChun commented Nov 5, 2024

eliorerz commented Nov 8, 2024

eliorerz commented Nov 10, 2024

openshift-ci bot commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 3, 2024 •

edited by openshift-ci bot

Loading

codecov bot commented Nov 3, 2024 •

edited

Loading

eliorerz commented Nov 3, 2024 •

edited

Loading