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

[NSXServiceAccount] Watch mgmt-proxy service and reconcile all NSXSA #1019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Atish-iaf
Copy link

@Atish-iaf Atish-iaf commented Feb 10, 2025

Reconcile all existing NSXSA incase of following events of K8s service corresponding to mgmt-proxy.

  • new service is created.
  • existing service is deleted.
  • existing service is updated with LB VIP.

While reconciling realized NSXSA, update proxyEndpoints if proxyEndpoints have changed.

Steps for testing -

  1. Build nsx-operator image and save it in a tar file.
    Run make photon in nsx-operator repo.
    docker image save github.com/vmware-tanzu/nsx-operator:latest -o my-nsx-operator.tar
  2. Get WCP testbed
  3. Copy nsx-operator image my-nsx-operator.tar file to SVCP VMs using scp
  4. Inside each SVCP VMs, run ctr -n=k8s.io images import <path to my-nsx-operator.tar> to load nsx-operator image.
  5. Update nsx-ncp deployment.
    kubectl set image deploy/nsx-ncp nsx-operator=github.com/vmware-tanzu/nsx-operator:latest -n vmware-system-nsx
  6. Create a guest cluster, nsxsa gets created with empty proxyEndpoints.
  7. register and install nsx-mgmt-proxy supervisor service.
  8. kubectl describe nsxsa, proxy endpoints get updated.
  9. Change proxy LB IP from VC UI by filling data-value loadBalancerIP: <someIP>, check in VC UI, it gets updated, then kubectl describe nsxsa and see proxyEndpoints get updated with new LB IP.

@edwardbadboy
Copy link

Thanks for the PR. Let me review it.

@Atish-iaf
Copy link
Author

Hi @edwardbadboy @gran-vmv @liu4480
Please help to review this PR.
Thanks

@Atish-iaf Atish-iaf force-pushed the watch_proxy_service_and_reconcile_nsxsa branch from d2a15ab to b982484 Compare February 11, 2025 05:47
Copy link
Contributor

@gran-vmv gran-vmv left a comment

Choose a reason for hiding this comment

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

Overall LGTM. A small comment.

@Atish-iaf Atish-iaf marked this pull request as ready for review February 12, 2025 04:59
@Atish-iaf Atish-iaf marked this pull request as draft February 18, 2025 09:48
@Atish-iaf Atish-iaf force-pushed the watch_proxy_service_and_reconcile_nsxsa branch from b982484 to d361378 Compare February 18, 2025 15:22
@Atish-iaf Atish-iaf marked this pull request as ready for review February 18, 2025 15:24
Copy link

@edwardbadboy edwardbadboy left a comment

Choose a reason for hiding this comment

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

Looks good. Only minor comments.

@Atish-iaf Atish-iaf force-pushed the watch_proxy_service_and_reconcile_nsxsa branch from d361378 to 4a2118e Compare February 20, 2025 14:11
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 27.71084% with 60 lines in your changes missing coverage. Please review.

Project coverage is 74.28%. Comparing base (2bd7c0e) to head (55c2f00).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../nsxserviceaccount/nsxserviceaccount_controller.go 23.28% 52 Missing and 4 partials ⚠️
pkg/nsx/services/nsxserviceaccount/cluster.go 60.00% 3 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (27.71%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
- Coverage   74.52%   74.28%   -0.24%     
==========================================
  Files         118      118              
  Lines       16330    16412      +82     
==========================================
+ Hits        12170    12192      +22     
- Misses       3391     3446      +55     
- Partials      769      774       +5     
Flag Coverage Δ
unit-tests 74.28% <27.71%> (-0.24%) ⬇️
Files with missing lines Coverage Δ
pkg/nsx/services/nsxserviceaccount/cluster.go 79.16% <60.00%> (-0.46%) ⬇️
.../nsxserviceaccount/nsxserviceaccount_controller.go 68.39% <23.28%> (-20.91%) ⬇️

edwardbadboy
edwardbadboy previously approved these changes Feb 21, 2025
@Atish-iaf
Copy link
Author

/e2e

1 similar comment
@Atish-iaf
Copy link
Author

/e2e

Reconcile all existing NSXSA incase of following events of K8s service
corresponding to mgmt-proxy :
- new service is created.
- existing service is deleted.
- existing service is updated with LB VIP.

While reconciling realized NSXSA, update proxyEndpoints only if it has changed.

Signed-off-by: Kumar Atish <[email protected]>
@Atish-iaf Atish-iaf force-pushed the watch_proxy_service_and_reconcile_nsxsa branch from 4a2118e to 55c2f00 Compare February 24, 2025 06:39
Copy link
Contributor

@liu4480 liu4480 left a comment

Choose a reason for hiding this comment

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

LGTM

@edwardbadboy
Copy link

Hello @Atish-iaf ,

Can you test this PR and describe how you test it in the PR description? Thanks!

@Atish-iaf
Copy link
Author

Atish-iaf commented Feb 27, 2025

Hello @Atish-iaf ,

Can you test this PR and describe how you test it in the PR description? Thanks!

Hi @edwardbadboy

  1. no proxy->nsxsa realized with empty proxyEndpoints->then proxy created->nsxsa got updated with proxy endpoints.
  2. change proxy LB IP via VC UI by filling data-values->realized nsxsa proxyEndpoints got updated with new proxy LB IP.
  3. delete proxy, nsxsa doesn't get updated, it still has old proxyEndpoints. ProxyEndpoints should be empty now ?
    Again create proxy, nsxsa get updated and replaces old proxyEndpoints.

Update
3. delete proxy, nsxsa proxyEndpoints become empty(it takes a little longer time to get updated after proxy delete event), Again create proxy, nsxsa gets updated with proxyEndpoints.

@liu4480
Copy link
Contributor

liu4480 commented Feb 28, 2025

Hello @Atish-iaf ,
Can you test this PR and describe how you test it in the PR description? Thanks!

Hi @edwardbadboy

1. no proxy->nsxsa realized with empty proxyEndpoints->then proxy created->nsxsa got updated with proxy endpoints.

2. change proxy LB IP via VC UI by filling data-values->realized nsxsa proxyEndpoints got updated with new proxy LB IP.

3. delete proxy, nsxsa doesn't get updated, it still has old proxyEndpoints. ProxyEndpoints should be empty now ?
   Again create proxy, nsxsa get updated and replaces old proxyEndpoints.

did you test this patch together with antrea-interworking change

@Atish-iaf
Copy link
Author

Atish-iaf commented Feb 28, 2025

Hello @Atish-iaf ,
Can you test this PR and describe how you test it in the PR description? Thanks!

Hi @edwardbadboy

1. no proxy->nsxsa realized with empty proxyEndpoints->then proxy created->nsxsa got updated with proxy endpoints.

2. change proxy LB IP via VC UI by filling data-values->realized nsxsa proxyEndpoints got updated with new proxy LB IP.

3. delete proxy, nsxsa doesn't get updated, it still has old proxyEndpoints. ProxyEndpoints should be empty now ?
   Again create proxy, nsxsa get updated and replaces old proxyEndpoints.

did you test this patch together with antrea-interworking change

Yes, register Pod detects the change but fails to update bootstrap-config map and register pod restarts.
I have updated it on interworking changes PR.

@edwardbadboy
Copy link

Thanks for the test I think this PR should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants