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.

Sign up for GitHub

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

Jump to bottom

Update NetworkConfiguration CR with new VPC API #595

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Update NetworkConfiguration CR with new VPC API #595

wants to merge 7 commits into from

Conversation

wenqiq
Copy link
Contributor

@wenqiq wenqiq commented Jun 4, 2024 •
edited
Loading

Change VPCNetworkConfiguration CR, add VPCConnectivityProfile ID.

@wenqiq wenqiq force-pushed the topic/wenqi/newVPCNetworkConfiguration branch 6 times, most recently from f27b714 to e45262d Compare June 4, 2024 08:58
@wenqiq wenqiq marked this pull request as ready for review June 4, 2024 09:06
@wenqiq wenqiq changed the title [Not4review]update NetworkConfiguration CR with new VPC API Update NetworkConfiguration CR with new VPC API Jun 4, 2024
@wenqiq wenqiq force-pushed the topic/wenqi/newVPCNetworkConfiguration branch 4 times, most recently from 88264d8 to 247ef00 Compare June 21, 2024 00:26
gran-vmv
gran-vmv reviewed Jun 25, 2024
View reviewed changes
pkg/apis/v1alpha1/vpcnetworkconfiguration_types.go
VPCConnectivityProfile string `json:"vpcConnectivityProfile,omitempty"`

// +kubebuilder:validation:Enum=SMALL;MEDIUM;LARGE;XLARGE
LbServiceSize string `json:"lbServiceSize,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add this to VPCNetworkConfigInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the LB size should be only configured in VPCNetworkConfiguration CR

Copy link
Contributor

Choose a reason for hiding this comment

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

VPCNetworkConfigInfo is an internal struct, and it is used to store VPC config.

@wenqiq wenqiq force-pushed the topic/wenqi/newVPCNetworkConfiguration branch from f195cb8 to d8b04e8 Compare June 25, 2024 06:49
seanpang-vmware
seanpang-vmware reviewed Jun 27, 2024
View reviewed changes
.gitignore
@@ -2,4 +2,6 @@
bin/
.DS_Store
go.work
go.work.sum
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not submit .gitignore

go.mod
@@ -43,6 +44,13 @@ require (
sigs.k8s.io/controller-runtime v0.16.0
)

require (
github.com/zhengxiexie/vsphere-automation-sdk-go/lib v0.7.2
Copy link
Contributor

Choose a reason for hiding this comment

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

do we merge this change now, or wait till new SDK ready and change it to official one?

go.mod Outdated
@@ -6,14 +6,15 @@ replace (
github.com/vmware-tanzu/nsx-operator/pkg/apis => ./pkg/apis
github.com/vmware-tanzu/nsx-operator/pkg/apis/v1alpha1 => ./pkg/apis/v1alpha1
github.com/vmware-tanzu/nsx-operator/pkg/apis/v1alpha2 => ./pkg/apis/v1alpha2
github.com/vmware-tanzu/nsx-operator/pkg/client => ./pkg/client
github.com/vmware-tanzu/nsx-operator/pkg/client => ./pkg/client
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, what changed on this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

space alignment

pkg/controllers/networkinfo/networkinfo_controller.go
@@ -41,6 +41,7 @@ type NetworkInfoReconciler struct {
Recorder record.EventRecorder
}

// Reconcile ...
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this comment do?

pkg/controllers/networkinfo/networkinfo_utils.go
@@ -5,7 +5,7 @@ import (
"fmt"
"reflect"

"github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model"
"github.com/zhengxiexie/vsphere-automation-sdk-go/services/nsxt/model"
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible we use replace in go.mod instead of changing this import path everywhere @TaoZou1 @wenqiq

dantingl
dantingl reviewed Jul 1, 2024
View reviewed changes
pkg/apis/v1alpha1/vpcnetworkconfiguration_types.go
VPCConnectivityProfile string `json:"vpcConnectivityProfile,omitempty"`

// +kubebuilder:validation:Enum=SMALL;MEDIUM;LARGE;XLARGE
LbServiceSize string `json:"lbServiceSize,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

No, the LB size should be only configured in VPCNetworkConfiguration CR

pkg/nsx/services/vpc/vpc.go
@@ -558,6 +558,15 @@ func (s *VPCService) CreateOrUpdateVPC(obj *v1alpha1.NetworkInfo) (*model.Vpc, *
return existingVPC[0], &nc, nil
}

if nc.VPCConnectivityProfile != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to create private IPBlock, you can just add private CIDR into VPC API request

Copy link
Contributor

Choose a reason for hiding this comment

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

Put this VPCConnectivityProfile logic into buildNSXVPC?

@dantingl
Copy link
Contributor

dantingl commented Jul 1, 2024

  1. please push a separate PR for CRD definition
  2. for VPC creation, no need to create private IPBlock, just need to set private cidr in VPC body

@wenqiq
update NetworkConfiguration CR with new VPC API
677a4e4 
Update sdk package

Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Change DefaultSubnetAccessMode to DefaultAccessModeForPod
5498c3d 
Add Project type

Add const AccessModeProject Project

Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Add LbServiceSize field in vpcnetworkconfiguration CR
a337095 
Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Change field name
52179f6 
Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Update package
d6222e0 
Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Add VPCConnectivityProfile
ed1c768 
Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Add LbServiceSize in VPCNetworkConfigInfo
93e9662 
Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>
@wenqiq
Copy link
Contributor Author

wenqiq commented Jul 1, 2024

A separate PR for CRD definition

#623

lxiaopei
lxiaopei reviewed Jul 4, 2024
View reviewed changes
pkg/apis/nsx.vmware.com/v1alpha1/vpcnetworkconfiguration_types.go
// Edge cluster path on which the networking elements will be created.
EdgeClusterPath string `json:"edgeClusterPath,omitempty"`

// NSX-T Project the Namespace associated with.
NSXTProject string `json:"nsxtProject,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

NSXTProject -> "NsxProject"

Copy link
Contributor

Choose a reason for hiding this comment

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

The VPCNetworkConfigurationSpec should be like
type VPCNetworkConfigurationSpec struct {
NsxProject
VpcConnectivityProfile
PrivateIPs
ShortID
LbServiceSize
PodSubnetAccessMode
DefaultSubnetSize
}

@wenqiq wenqiq force-pushed the topic/wenqi/newVPCNetworkConfiguration branch from d8b04e8 to 93e9662 Compare July 9, 2024 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantingl dantingl dantingl left review comments

@lxiaopei lxiaopei lxiaopei left review comments

@gran-vmv gran-vmv gran-vmv left review comments

@seanpang-vmware seanpang-vmware seanpang-vmware left review comments

@TaoZou1 TaoZou1 Awaiting requested review from TaoZou1

@zhengxiexie zhengxiexie Awaiting requested review from zhengxiexie

@liu4480 liu4480 Awaiting requested review from liu4480

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wenqiq @dantingl @lxiaopei @gran-vmv @seanpang-vmware @vmwclabot