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

added the cluster id inside deployment controller #176

Merged
merged 3 commits into from
May 11, 2021
Merged

added the cluster id inside deployment controller #176

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
697b187
added the cluster id inside deployment controller
Mengying-Li May 8, 2021
4dee175
fixed the helm setup failure
Mengying-Li May 8, 2021
ab2056a
added cluster id in other remote controllers
Mengying-Li May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ jobs:
curl -Lo minikube https://github.com/kubernetes/minikube/releases/download/${MINIKUBE_VERSION}/minikube-linux-amd64 && chmod +x minikube && sudo mv minikube /usr/local/bin/
- run:
name: setup helm
command: curl https://raw.githubusercontent.com/helm/helm/master/scripts/get | bash
command: curl -fsSL https://raw.githubusercontent.com/helm/helm/master/scripts/get | bash -s -- -v v2.17.0
- run:
name: Set up kustomize
command: |
Expand Down
2 changes: 1 addition & 1 deletion admiral/pkg/clusters/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste
}

log.Infof("starting deployment controller clusterID: %v", clusterID)
rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r}, clientConfig, resyncPeriod)
rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other controllers missing ClusterId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe so. I checked for other controllers, there are about over half that don't have cluster id passed in:
NewGlobalTrafficController
NewDeploymentController( fixed)
NewRolloutsController
NewPodController
NewNodeController
NewServiceController

Link to registry file: https://github.com/istio-ecosystem/admiral/blob/master/admiral/pkg/clusters/registry.go#L125.
But all SE, DR, VS related ones have clusterID passed in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you like me to update all of them ? @aattuluri

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that would be great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done @aattuluri Only controller I didn't add cluster id yet is dependency controller. My understanding is that dependency controller is not a remote cluster controller so it only have one that live in the same cluster where admiral lives in. So not too sure if a cluster id is needed for this one. (And looks a bit different from implementation wise for cluster id to go in compare with the other controller's cluster id so I didn't make any changes here for now) Let me know if you feel otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that one is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other controllers missing ClusterId?


if err != nil {
return fmt.Errorf(" Error with DeploymentController controller init: %v", err)
Expand Down
4 changes: 3 additions & 1 deletion admiral/pkg/clusters/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ func (g *globalTrafficCache) Delete(gtp *v1.GlobalTrafficPolicy) {

type DeploymentHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type PodHandler struct {
Expand Down Expand Up @@ -359,6 +360,7 @@ func (gtp *GlobalTrafficHandler) Deleted(obj *v1.GlobalTrafficPolicy) {
}

func (pc *DeploymentHandler) Added(obj *k8sAppsV1.Deployment) {
obj.ClusterName = pc.ClusterID
HandleEventForDeployment(admiral.Add, obj, pc.RemoteRegistry)
}

Expand Down Expand Up @@ -445,7 +447,7 @@ func HandleEventForDeployment(event admiral.EventType, obj *k8sAppsV1.Deployment
globalIdentifier := common.GetDeploymentGlobalIdentifier(obj)

if len(globalIdentifier) == 0 {
log.Infof(LogFormat, "Event", "deployment", obj.Name, "", "Skipped as '"+common.GetWorkloadIdentifier()+" was not found', namespace="+obj.Namespace)
log.Infof(LogFormat, "Event", "deployment", obj.Name, obj.ClusterName, "Skipped as '"+common.GetWorkloadIdentifier()+" was not found', namespace="+obj.Namespace)
return
}

Expand Down