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

Cleanup addons logic. #1946

Merged
merged 1 commit into from Dec 21, 2017
Merged

Cleanup addons logic. #1946

merged 1 commit into from Dec 21, 2017

Conversation

JunSun17
Copy link
Collaborator

@JunSun17 JunSun17 commented Dec 19, 2017

What this PR does / why we need it: Centralize the addon related code, simplify the code change needed on dev side to add a new addon.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes # Part of #1869

Special notes for your reviewer:

Centralize the addon related code in addons.go. To add a new addon:

  1. an addon config yaml file needs to be created and being put under k8s/addon directory.
  2. add the new addon to the list kubernetesAddonSettings in addons.go. The list entry is like this:
kubernetesAddonSetting{
    "kubernetesmasteraddons-kubernetes-dashboard-deployment.yaml", // addon yaml file created
    profile.OrchestratorProfile.KubernetesConfig.IsDashboardEnabled(), // toggle logic to enable addon
    "kubernetes-dashboard-deployment.yaml", // the file name which will be copied to master nodes
},

Then code should pickup the addon and write proper statements into the kubernetesmastercustomerdata.yaml.

Also moved all 1.5 version addon yamls to k8s/addon/1.5 directory. The code will pick the proper versioned addon files based on the input OrchestratorVersion. No hard-coding of version number in the code.

manifests and artifacts can be treated the same way if needed.

Release note:

@ghost ghost assigned JunSun17 Dec 19, 2017
@ghost ghost added the in progress label Dec 19, 2017
@JunSun17 JunSun17 force-pushed the addons-management branch 8 times, most recently from 3c95bf3 to 518a6cd Compare December 21, 2017 00:08
jackfrancis
jackfrancis previously approved these changes Dec 21, 2017
Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@jackfrancis jackfrancis merged commit afbf71b into Azure:master Dec 21, 2017
@ghost ghost removed the in progress label Dec 21, 2017
@JunSun17 JunSun17 deleted the addons-management branch December 21, 2017 23:07
@karataliu
Copy link
Contributor

A file named 'parts/k8s/addons/1.6/kubernetesmasteraddons-calico-daemonset1.5.yaml' was added,
as it is ported from

if profile.OrchestratorProfile.KubernetesConfig.NetworkPolicy == "calico" {		
				if strings.HasPrefix(profile.OrchestratorProfile.OrchestratorVersion, "1.5.") ||		
					strings.HasPrefix(profile.OrchestratorProfile.OrchestratorVersion, "1.6.") {		
					calicoAddonYamls = calicoAddonYamls15

This still looks a bit strange, do we consider dropping version suffixes for files under versioned dir?

@karataliu
Copy link
Contributor

Another issue is 'filepath.Walk' makes it dependent on current file system. That is to say, loading versioned artifacts only works when we are on root path of the project. Using a standalone 'acs-engine' release no longer work, unless we copy 'parts/k8s/...' dir structs there. This seems go against previous behavior.

e.g.
I tried to add a 1.8 dir with dashboard yaml, the outcome is different when running on different dirs:
Not at project root dir

error: "lstat parts/k8s/addons/1.8: no such file or directory"
got: k8s/addons/kubernetesmasteraddons-kubernetes-dashboard-deployment.yaml

At project root dir

error: %!q(<nil>)
got: k8s/addons/1.8/kubernetesmasteraddons-kubernetes-dashboard-deployment.yaml

@JunSun17
Copy link
Collaborator Author

A file named 'parts/k8s/addons/1.6/kubernetesmasteraddons-calico-daemonset1.5.yaml' was added,
as it is ported from

if profile.OrchestratorProfile.KubernetesConfig.NetworkPolicy == "calico" {
if strings.HasPrefix(profile.OrchestratorProfile.OrchestratorVersion, "1.5.") ||
strings.HasPrefix(profile.OrchestratorProfile.OrchestratorVersion, "1.6.") {
calicoAddonYamls = calicoAddonYamls15
This still looks a bit strange, do we consider dropping version suffixes for files under versioned dir?

Yes, these version suffixes can definitely be dropped. I have not done that yet since I want this PR to show exactly how files are moved. I will do it in a later PR.

@JunSun17
Copy link
Collaborator Author

Another issue is 'filepath.Walk' makes it dependent on current file system. That is to say, loading versioned artifacts only works when we are on root path of the project. Using a standalone 'acs-engine' release no longer work, unless we copy 'parts/k8s/...' dir structs there. This seems go against previous behavior.

e.g.
I tried to add a 1.8 dir with dashboard yaml, the outcome is different when running on different dirs:
Not at project root dir

error: "lstat parts/k8s/addons/1.8: no such file or directory"
got: k8s/addons/kubernetesmasteraddons-kubernetes-dashboard-deployment.yaml
At project root dir

error: %!q()
got: k8s/addons/1.8/kubernetesmasteraddons-kubernetes-dashboard-deployment.yaml

Yes, I take the assumption that acs-engine executable is always run from root. I will look into this issue and have it updated in the next PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants