Skip to content
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

requeue reconcile for klusterlet secrets #140

Merged
merged 4 commits into from
Jan 3, 2023
Merged

requeue reconcile for klusterlet secrets #140

merged 4 commits into from
Jan 3, 2023

Conversation

rokej
Copy link
Contributor

@rokej rokej commented Jan 3, 2023

Signed-off-by: Roke Jung roke@redhat.com

Description of the change(s):

  • When the agent reconcile fails to create external-managed-kubeconfig secret in the hosted cluster's klusterlet namespace or hub mirror secret, requeue the reconcile until it succeeds.

Why do we need this PR:

  • This fixes the current assumption that the klusterlet is available when the hosted control plane becomes available.
  • This fixes the current assumption that a managed cluster is created before a hosted cluster.

Issue reference:

Test API/Unit - Success

KUBEBUILDER_ASSETS="/Users/rokej/Library/Application Support/io.kubebuilder.envtest/k8s/1.22.1-darwin-amd64" go test github.com/stolostron/hypershift-addon-operator/cmd github.com/stolostron/hypershift-addon-operator/pkg/agent github.com/stolostron/hypershift-addon-operator/pkg/install github.com/stolostron/hypershift-addon-operator/pkg/manager github.com/stolostron/hypershift-addon-operator/pkg/metrics github.com/stolostron/hypershift-addon-operator/pkg/util -coverprofile cover.out
?   	github.com/stolostron/hypershift-addon-operator/cmd	[no test files]
ok  	github.com/stolostron/hypershift-addon-operator/pkg/agent	15.990s	coverage: 71.6% of statements
ok  	github.com/stolostron/hypershift-addon-operator/pkg/install	147.267s	coverage: 86.3% of statements
ok  	github.com/stolostron/hypershift-addon-operator/pkg/manager	1.341s	coverage: 56.0% of statements
ok  	github.com/stolostron/hypershift-addon-operator/pkg/metrics	0.541s	coverage: 100.0% of statements
?   	github.com/stolostron/hypershift-addon-operator/pkg/util	[no test files]

Signed-off-by: Roke Jung <roke@redhat.com>
@openshift-ci openshift-ci bot added the approved label Jan 3, 2023
@rokej rokej requested a review from mikeshng January 3, 2023 19:05
Copy link
Contributor

@mikeshng mikeshng left a comment

Choose a reason for hiding this comment

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

Just some minor feedback. Good otherwise.

pkg/agent/agent.go Outdated Show resolved Hide resolved
pkg/agent/agent.go Outdated Show resolved Hide resolved
pkg/agent/agent.go Outdated Show resolved Hide resolved
pkg/agent/agent.go Outdated Show resolved Hide resolved
Signed-off-by: Roke Jung <roke@redhat.com>
Copy link
Contributor

@mikeshng mikeshng left a comment

Choose a reason for hiding this comment

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

/approve

/lgtm

Signed-off-by: Roke Jung <roke@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Jan 3, 2023
@mikeshng
Copy link
Contributor

mikeshng commented Jan 3, 2023

@rokej I suggested this line

c.log.Error(err, "failed to find the klusterlet namespace: %s", klusterletNamespaceNsn.Name)

based on the other c.log.Error lines in the function. I am guessing they are all invalid and we just never ran into them because we didn't have test or run into them during runtime.

I think we should fix them all. Maybe something like this:

c.log.Error(err, fmt.Sprintf("failed to find the klusterlet namespace: %s", klusterletNamespaceNsn.Name))

or

c.log.Error(err, "failed to find the klusterlet namespace: " + klusterletNamespaceNsn.Name)

Signed-off-by: Roke Jung <roke@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Jan 3, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

44.4% 44.4% Coverage
0.0% 0.0% Duplication

@openshift-ci
Copy link

openshift-ci bot commented Jan 3, 2023

@rokej: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sonar fb4d03b link true /test sonar

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/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@mikeshng mikeshng left a comment

Choose a reason for hiding this comment

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

/approve

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 3, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jan 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikeshng, rokej

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rokej
Copy link
Contributor Author

rokej commented Jan 3, 2023

Cannot cover log message tests. Force merging.

@rokej rokej merged commit 6844934 into stolostron:main Jan 3, 2023
@rokej rokej deleted the ACM-2208 branch January 3, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants