-
Notifications
You must be signed in to change notification settings - Fork 558
Updates from aks-engine spike #4302
Changes from all commits
dc6d3c0
3858592
db00994
d4247ef
4629ab8
8381a67
aa0d496
e54d30b
07a85e9
aee6c35
4975d70
871a965
7811dd1
39567a1
0129ac9
b01e246
58918c0
ca38dfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
log_level: debug | ||
|
||
tide: | ||
# target_url: http://ci-bot-aks-ingress.eastus.cloudapp.azure.com/tide.html | ||
# target_url: http://prow-ci-bot-ingress.eastus.cloudapp.azure.com/tide.html | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the new prow CI bot ready for acs-engine (not just aks-engine)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no this bot runs for aks-engine, not acs-engine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed the setup to make it work automatically so we do want these changes though |
||
merge_method: | ||
Azure/acs-engine: squash | ||
queries: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ DIST_DIRS = find * -type d -exec | |
|
||
.NOTPARALLEL: | ||
|
||
.PHONY: bootstrap build test test_fmt validate-generated fmt lint ci devenv | ||
.PHONY: bootstrap build test test_fmt fmt lint ci devenv | ||
|
||
ifdef DEBUG | ||
GOFLAGS := -gcflags="-N -l" | ||
|
@@ -25,7 +25,7 @@ GITTAG := $(VERSION_SHORT) | |
endif | ||
|
||
REPO_PATH := github.com/Azure/acs-engine | ||
DEV_ENV_IMAGE := quay.io/deis/go-dev:v1.17.2 | ||
DEV_ENV_IMAGE := quay.io/deis/go-dev:v1.17.3 | ||
DEV_ENV_WORK_DIR := /go/src/${REPO_PATH} | ||
DEV_ENV_OPTS := --rm -v ${CURDIR}:${DEV_ENV_WORK_DIR} -w ${DEV_ENV_WORK_DIR} ${DEV_ENV_VARS} | ||
DEV_ENV_CMD := docker run ${DEV_ENV_OPTS} ${DEV_ENV_IMAGE} | ||
|
@@ -44,10 +44,6 @@ all: build | |
dev: | ||
$(DEV_ENV_CMD_IT) bash | ||
|
||
.PHONY: validate-generated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should stay in acs-engine until OpenShift support is removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, because OpenShift isn't active, this build task is already obsolete. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok to remove |
||
validate-generated: bootstrap | ||
./scripts/validate-generated.sh | ||
|
||
.PHONY: validate-dependencies | ||
validate-dependencies: bootstrap | ||
./scripts/validate-dependencies.sh | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
$REPO_PATH = "github.com/Azure/acs-engine" | ||
$DEV_ENV_IMAGE = "quay.io/deis/go-dev:v1.17.2" | ||
$DEV_ENV_IMAGE = "quay.io/deis/go-dev:v1.17.3" | ||
$DEV_ENV_WORK_DIR = "/go/src/$REPO_PATH" | ||
|
||
docker.exe run -it --rm -w $DEV_ENV_WORK_DIR -v `"$($PWD)`":$DEV_ENV_WORK_DIR $DEV_ENV_IMAGE bash |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,15 +195,6 @@ | |
}, | ||
"type": "string" | ||
}, | ||
{{if not IsHostedMaster}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this section need to stay here to support ACS use cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This removal is part of the addons refactor. @tariq1890 can verify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I can confirm |
||
"kubernetesNonMasqueradeCidr": { | ||
"metadata": { | ||
"description": "kubernetesNonMasqueradeCidr cluster subnet" | ||
}, | ||
"defaultValue": "{{GetDefaultVNETCIDR}}", | ||
"type": "string" | ||
}, | ||
{{end}} | ||
"kubernetesKubeletClusterDomain": { | ||
"metadata": { | ||
"description": "--cluster-domain Kubelet config" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,17 +96,6 @@ func assignKubernetesParameters(properties *api.Properties, parametersMap params | |
CloudProviderRateLimitBucket: kubernetesConfig.CloudProviderRateLimitBucket, | ||
}) | ||
addValue(parametersMap, "kubeClusterCidr", kubernetesConfig.ClusterSubnet) | ||
if !properties.IsHostedMasterProfile() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also same as above w/ respect to addons refactor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed |
||
if properties.OrchestratorProfile.IsAzureCNI() { | ||
if properties.MasterProfile != nil && properties.MasterProfile.IsCustomVNET() { | ||
addValue(parametersMap, "kubernetesNonMasqueradeCidr", properties.MasterProfile.VnetCidr) | ||
} else { | ||
addValue(parametersMap, "kubernetesNonMasqueradeCidr", DefaultVNETCIDR) | ||
} | ||
} else { | ||
addValue(parametersMap, "kubernetesNonMasqueradeCidr", properties.OrchestratorProfile.KubernetesConfig.ClusterSubnet) | ||
} | ||
} | ||
addValue(parametersMap, "kubernetesKubeletClusterDomain", kubernetesConfig.KubeletConfig["--cluster-domain"]) | ||
addValue(parametersMap, "dockerBridgeCidr", kubernetesConfig.DockerBridgeSubnet) | ||
addValue(parametersMap, "networkPolicy", kubernetesConfig.NetworkPolicy) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -534,16 +534,13 @@ func (t *TemplateGenerator) getTemplateFuncMap(cs *api.ContainerService) templat | |
if cs.Properties.OrchestratorProfile.OrchestratorType == api.DCOS { | ||
return helpers.GetDCOSMasterAllowedSizes() | ||
} | ||
return helpers.GetMasterAgentAllowedSizes() | ||
return helpers.GetKubernetesAllowedSizes() | ||
}, | ||
"GetDefaultVNETCIDR": func() string { | ||
return DefaultVNETCIDR | ||
}, | ||
"GetAgentAllowedSizes": func() string { | ||
if cs.Properties.OrchestratorProfile.IsKubernetes() || cs.Properties.OrchestratorProfile.IsOpenShift() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this change is safe to merge, but ideally it would be part of a separate "remove OpenShift" PR. |
||
return helpers.GetKubernetesAgentAllowedSizes() | ||
} | ||
return helpers.GetMasterAgentAllowedSizes() | ||
return helpers.GetKubernetesAllowedSizes() | ||
}, | ||
"getSwarmVersions": func() string { | ||
return getSwarmVersions(api.SwarmVersion, api.SwarmDockerComposeVersion) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed because OpenShift support was taken out of aks-engine, but I think it should stay in acs-engine. (For now--I'm going to remove OpenShift support separately in a later PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CecileRobertMichon thoughts? Because we don't run OpenShift tests anymore this is fine to take out preemptively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok with me, I'm just complaining that the PR would be simpler without this removal, and that the OpenShift code removal contained herein is incomplete, so if we merge this I'll follow up with another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was removed in aks-engine. I remember doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it either way, I would have preferred removing it with the PR that removes the relevant code but unit tests passed already so this is fine too.