Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Configure avi node network list in Cluster Class #4156

Merged
merged 2 commits into from
Jan 4, 2023
Merged

Configure avi node network list in Cluster Class #4156

merged 2 commits into from
Jan 4, 2023

Conversation

chenlin07
Copy link
Contributor

@chenlin07 chenlin07 commented Dec 13, 2022 •
edited
Loading

What this PR does / why we need it

In Cluster Class, we need to support to configure the required avi node network list var.

Previously we support this using ytt template: https://github.com/vmware-tanzu/tanzu-framework/blob/main/providers/ytt/02_addons/avi/ako_operator_secret.yaml#L6-L14

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Manual test. The avi_ingress_node_network_list will be auto-filled if the value is empty

Release note

User can configure `AVI_INGRESS_NODE_NETWORK_LIST` in cluster configuration

Additional information

Special notes for your reviewer

@chenlin07 chenlin07 requested review from a team as code owners December 13, 2022 02:53
@chenlin07
Copy link
Contributor Author

/test install-vc7

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221213030220/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@alfredthenarwhal
Copy link
Collaborator

@chenlin07: /test install-vc7
Commit: 603248a

Tests failed! Build no: 3573

@chenlin07 chenlin07 changed the title [WIP]Validate and auto-fill avi node network list Configure avi node network list in Cluster Class Dec 15, 2022
@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 603248a to 654c284 Compare December 16, 2022 01:01
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221216010845/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Dec 16, 2022 •
edited
Loading

Codecov Report

Merging #4156 (1e4905d) into main (6652c70) will decrease coverage by 0.87%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##             main    #4156      +/-   ##
==========================================
- Coverage   49.31%   48.43%   -0.88%     
==========================================
  Files         451      481      +30     
  Lines       44602    46749    +2147     
==========================================
+ Hits        21996    22644     +648     
- Misses      20530    21978    +1448     
- Partials     2076     2127      +51
Impacted Files Coverage Δ
tkg/managementcomponents/helper.go 93.63% <92.50%> (+3.33%) ⬆️
tkg/client/management_components.go 31.39% <100.00%> (+0.15%) ⬆️
...oller-manager/controllers/v3_cascade_controller.go 66.66% <0.00%> (-10.00%) ⬇️
.../cli/plugin/cluster/get_machinehealthcheck_node.go 9.30% <0.00%> (ø)
.../isolated-cluster/imgpkginterface/client_imgpkg.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/delete.go 12.50% <0.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
.../cli/plugin/cluster/set_machinehealthcheck_node.go 23.33% <0.00%> (ø)
...li/plugin/isolated-cluster/imgpkginterface/util.go 50.00% <0.00%> (ø)
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch 2 times, most recently from b445a3f to 3fbd9f4 Compare December 20, 2022 01:37
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221220013802/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 3fbd9f4 to 1e0dd92 Compare December 20, 2022 01:40
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221220014827/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221220014833/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 1e0dd92 to 10491f6 Compare December 20, 2022 05:42
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221220054945/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

XudongLiuHarold
XudongLiuHarold approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@XudongLiuHarold XudongLiuHarold left a comment

Choose a reason for hiding this comment

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

Generally LGTM, thx @chenlin07

tkg/managementcomponents/helper.go Outdated
},
}
}
return nodeNetworkList, nil
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we move this return line to line 153 above?

else {
		// return vsphere network if node network list is not set
		network_pathes := strings.Split(convertToString(userProviderConfigValues[constants.ConfigVariableVsphereNetwork]), "/")
		network := network_pathes[len(network_pathes)-1]
		nodeNetworkList = []NodeNetwork{
			NodeNetwork{
				NetworkName: network,
			},
		}
	return nodeNetworkList, nil
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there will be error if we don't have one return line outside of if else

tkg/managementcomponents/helper.go Outdated Show resolved Hide resolved
@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 10491f6 to 260a18f Compare December 21, 2022 03:35
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20221221034151/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 260a18f to 17c9a05 Compare January 3, 2023 07:50
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20230103075729/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20230103085946/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chenlin07 chenlin07 force-pushed the topic/chenlin/add-nodenetwork-list branch from 486e20c to 1e4905d Compare January 3, 2023 09:31
XudongLiuHarold
XudongLiuHarold approved these changes Jan 3, 2023
View reviewed changes
Copy link
Member

@XudongLiuHarold XudongLiuHarold left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @chenlin07

@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4156/20230103093802/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@randomvariable randomvariable added the ok-to-merge PRs should be labelled with this before merging label Jan 4, 2023
@randomvariable randomvariable merged commit b514752 into main Jan 4, 2023
@randomvariable randomvariable deleted the topic/chenlin/add-nodenetwork-list branch January 4, 2023 10:09
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@XudongLiuHarold XudongLiuHarold XudongLiuHarold approved these changes

Assignees
No one assigned
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chenlin07 @alfredthenarwhal @XudongLiuHarold @randomvariable @vmwclabot