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

🐛 Failed to sync sa work-controller-sa in cluster manger hosted mode #223

Merged

Conversation

zhujian7
Copy link
Member

@zhujian7 zhujian7 commented Jul 14, 2023

Summary

  1. Fix issue for cluster manger in hosted mode:
apiVersion: operator.open-cluster-management.io/v1
kind: ClusterManager
metadata:
  name: cluster-manager
spec:
  addOnManagerImagePullSpec: quay.io/open-cluster-management/addon-manager:latest
  deployOption:
    hosted:
      registrationWebhookConfiguration:
        address: management-control-plane
        port: 30443
      workWebhookConfiguration:
        address: management-control-plane
        port: 31443
    mode: Hosted
  placementImagePullSpec: quay.io/open-cluster-management/placement:latest
  registrationImagePullSpec: quay.io/open-cluster-management/registration:latest
  workImagePullSpec: quay.io/open-cluster-management/work:latest
status:
  conditions:
  - lastTransitionTime: "2023-07-14T07:51:04Z"
    message: 'Failed to sync service account: serviceaccounts "work-controller-sa"
      not found'
    reason: ServiceAccountSyncFailed
    status: "False"
    type: Applied

cluster-manager logs:

E0714 07:51:54.910585       1 base_controller.go:270] "ClusterManagerController" controller failed to sync "cluster-manager", err: serviceaccounts "work-controller-sa" not found
E0714 07:52:37.108738       1 base_controller.go:270] "ClusterManagerController" controller failed to sync "cluster-manager", err: serviceaccounts "work-controller-sa" not found
E0714 07:54:00.264383       1 base_controller.go:270] "ClusterManagerController" controller failed to sync "cluster-manager", err: serviceaccounts "work-controller-sa" not found
  1. add permission for cluster manager to get secret addon-manager-controller-sa-kubeconfig

  2. fix flaky integration test

Related issue(s)

Fixes #

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (9ff0948) 60.26% compared to head (5190184) 60.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
- Coverage   60.26%   60.22%   -0.04%     
==========================================
  Files         130      130              
  Lines       13494    13502       +8     
==========================================
  Hits         8132     8132              
- Misses       4611     4619       +8     
  Partials      751      751              
Flag Coverage Δ
unit 60.22% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...stermanagercontroller/clustermanager_controller.go 54.14% <0.00%> (ø)
...agercontroller/clustermanager_runtime_reconcile.go 50.42% <0.00%> (-3.64%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@openshift-ci openshift-ci bot removed the approved label Jul 14, 2023
@@ -55,7 +55,8 @@ type clusterManagerController struct {
cache resourceapply.ResourceCache
// For testcases which don't need these functions, we could set fake funcs
ensureSAKubeconfigs func(ctx context.Context, clusterManagerName, clusterManagerNamespace string,
hubConfig *rest.Config, hubClient, managementClient kubernetes.Interface, recorder events.Recorder) error
hubConfig *rest.Config, hubClient, managementClient kubernetes.Interface, recorder events.Recorder,
mwctrEnabled, addonManagerEnabled bool) error
Copy link
Member

Choose a reason for hiding this comment

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

can we just use features? it is global variable, we do not need them as input

Copy link
Member Author

@zhujian7 zhujian7 Jul 14, 2023

Choose a reason for hiding this comment

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

for hosted mode, there might be multiple clusterManager CRs, if they have different feature gate settings, I don't think using the global variable is appropriate.

@qiujian16
Copy link
Member

also I think we need to enhance the integration test for this bug?

Signed-off-by: zhujian <jiazhu@redhat.com>
@zhujian7
Copy link
Member Author

cc @xuezhaojun @zhiweiyin318

@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhujian7

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

@openshift-merge-robot openshift-merge-robot merged commit 8d974c2 into open-cluster-management-io:main Jul 17, 2023
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.

3 participants