-
Notifications
You must be signed in to change notification settings - Fork 560
Conversation
63e9936
to
ee14ae9
Compare
@sozercan Thanks for adding this. We may only apply the options for master nodes, as they're only used in kube-controller-manager or cloud-controller-manager. |
@feiskyer i can add it to |
Yep, I see. It's also ok to set it in node's configs. |
Codecov Report
@@ Coverage Diff @@
## master #3515 +/- ##
==========================================
- Coverage 55.46% 55.39% -0.07%
==========================================
Files 108 108
Lines 16069 16098 +29
==========================================
+ Hits 8912 8918 +6
- Misses 6389 6413 +24
+ Partials 768 767 -1 |
docs/clusterdefinition.md
Outdated
| serviceCidr | no | IP range for Service IPs, Default is "10.0.0.0/16". This range is never routed outside of a node so does not need to lie within clusterSubnet or the VNET | | ||
| useInstanceMetadata | no | Use the Azure cloudprovider instance metadata service for appropriate resource discovery operations. Default is `true` | | ||
| useManagedIdentity | no | Includes and uses MSI identities for all interactions with the Azure Resource Manager (ARM) API. Instead of using a static service principal written to /etc/kubernetes/azure.json, Kubernetes will use a dynamic, time-limited token fetched from the MSI extension running on master and agent nodes. This support is currently alpha and requires Kubernetes v1.9.1 or newer. (boolean - default == false) | | ||
| loadBalancerSku | no | Sku of Load Balancer and Public IP. Candidate values are: `basic` and `standard`. If not set, it will be default to basic. Requires Kubernetes 1.11 or newer. | |
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.
Only standard requires 1.11 correct? "standard
requires Kubernetes 1.11 or newer" might be more clear
docs/clusterdefinition.md
Outdated
| useInstanceMetadata | no | Use the Azure cloudprovider instance metadata service for appropriate resource discovery operations. Default is `true` | | ||
| useManagedIdentity | no | Includes and uses MSI identities for all interactions with the Azure Resource Manager (ARM) API. Instead of using a static service principal written to /etc/kubernetes/azure.json, Kubernetes will use a dynamic, time-limited token fetched from the MSI extension running on master and agent nodes. This support is currently alpha and requires Kubernetes v1.9.1 or newer. (boolean - default == false) | | ||
| loadBalancerSku | no | Sku of Load Balancer and Public IP. Candidate values are: `basic` and `standard`. If not set, it will be default to basic. Requires Kubernetes 1.11 or newer. | | ||
| excludeMasterFromStandardLB | no | Excludes master nodes from standard load balancer. Default is `true`. Requires Kubernetes 1.11 or newer. | |
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.
Same thing here, which part requires 1.11? If the default is always true then what happens if the user is on k8s 1.8?
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'll just be ignored by k8s v1.10 or earlier (since anything but v1.11 doesn't check for those values). @feiskyer can you confirm?
@@ -65,6 +65,8 @@ $global:KubeDnsSearchPath = "svc.cluster.local" | |||
|
|||
$global:UseManagedIdentityExtension = "{{WrapAsVariable "useManagedIdentityExtension"}}" | |||
$global:UseInstanceMetadata = "{{WrapAsVariable "useInstanceMetadata"}}" | |||
$global:LoadBalancerSku = "{{WrapAsVariable "loadBalancerSku"}}" |
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.
@PatrickLang could you please review this file?
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.
Sure - the changes here look good to me. I don't see any Windows-specific things changing. The only questions from me are whether this needs an azure-cni update and if the cloud provider already supports these in K8s 1.9 - 1.11
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 code wouldn't need to change, just checking that those are updated if required when this is merged
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.
@PatrickLang k8s v1.11 is required for this, tested this on windows pool and it successfully created standard lb and public ip.
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.
Thanks @PatrickLang
d4d1f7d
to
1ee0c6a
Compare
docs/clusterdefinition.md
Outdated
| useInstanceMetadata | no | Use the Azure cloudprovider instance metadata service for appropriate resource discovery operations. Default is `true` | | ||
| useManagedIdentity | no | Includes and uses MSI identities for all interactions with the Azure Resource Manager (ARM) API. Instead of using a static service principal written to /etc/kubernetes/azure.json, Kubernetes will use a dynamic, time-limited token fetched from the MSI extension running on master and agent nodes. This support is currently alpha and requires Kubernetes v1.9.1 or newer. (boolean - default == false) | | ||
| loadBalancerSku | no | Sku of Load Balancer and Public IP. Candidate values are: `basic` and `standard`. If not set, defaults to `basic`. `standard` requires Kubernetes 1.11 or newer. | | ||
| excludeMasterFromStandardLB | no | Excludes master nodes from standard load balancer. Requires `loadBalancerSku` to be set to `standard` and Kubernetes v1.11 or newer. Default is `true`. | |
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 still think that this default behavior is counter intuitive since we are always defaulting to true even though that is only supported for k8s 1.11 with standard LB.... which means that we are defaulting to a non-supported option
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.
should we flip around the setting to includeMasterInStandardLB
? @feiskyer thoughts?
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.
Could we set excludeMasterFromStandardLB
only when it is configured explicitly by the user? The default true
is recommended.
includeMasterInStandardLB
is actually same with excludeMasterFromStandardLB
, just with a different default value. Since it's a k8s configure option, it's better to keep it as is.
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.
Could the default behavior be: if loadBalancerSku == standard => true, Else => false
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.
Windows change looks good to me. The tests in place do cover ingress to a Windows service so be sure to update the apimodel to test each mode before merge
@@ -65,6 +65,8 @@ $global:KubeDnsSearchPath = "svc.cluster.local" | |||
|
|||
$global:UseManagedIdentityExtension = "{{WrapAsVariable "useManagedIdentityExtension"}}" | |||
$global:UseInstanceMetadata = "{{WrapAsVariable "useInstanceMetadata"}}" | |||
$global:LoadBalancerSku = "{{WrapAsVariable "loadBalancerSku"}}" |
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.
Sure - the changes here look good to me. I don't see any Windows-specific things changing. The only questions from me are whether this needs an azure-cni update and if the cloud provider already supports these in K8s 1.9 - 1.11
@@ -65,6 +65,8 @@ $global:KubeDnsSearchPath = "svc.cluster.local" | |||
|
|||
$global:UseManagedIdentityExtension = "{{WrapAsVariable "useManagedIdentityExtension"}}" | |||
$global:UseInstanceMetadata = "{{WrapAsVariable "useInstanceMetadata"}}" | |||
$global:LoadBalancerSku = "{{WrapAsVariable "loadBalancerSku"}}" |
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 code wouldn't need to change, just checking that those are updated if required when this is merged
I looked at the section @CecileRobertMichon recommended, but didn't review the rest |
cc @khenidak |
code lgtm, pending rebase and E2E |
1ee0c6a
to
936c319
Compare
@jackfrancis rebased |
@khenidak yup |
pkg/acsengine/defaults.go
Outdated
} | ||
|
||
if a.OrchestratorProfile.KubernetesConfig.ExcludeMasterFromStandardLB == nil { | ||
a.OrchestratorProfile.KubernetesConfig.ExcludeMasterFromStandardLB = helpers.PointerToBool(api.DefaultExcludeMasterFromStandardLB) |
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.
As @CecileRobertMichon suggested, let's convert the default value assignment of ExcludeMasterFromStandardLB
to a function. The function should look like this:
- is this a 1.11 or greater cluster that is using "Standard" LB?
- if so, set to true (based on comments from @feiskyer)
- if not, false
936c319
to
80d1fdb
Compare
Our E2E tests tell us that ILB scenarios break w/ standard LB-enabled clusters. @feiskyer do you have an upstream bug that this may be related to? |
This is a prereq for #3453. |
Which region are you using? Could you verify whether it's a Standard LB issue (e.g. without k8s and enabling floatingIP)? I have met that before and standard LB with floatingIP enabled may not work on some regions. |
@feiskyer acs-engine CI failed on southcentralus, westcentralus, southeastasia, westeurope (it failed on all 4 runs) This is the test that fails which tests working ILB: |
@sozercan Thanks. So it's a standard ILB issue, which disables external connectivity by default. |
Hold, we are in talks with Networking team for clarity on what we can do. Should resolve this week |
9ccbee1
to
dcb3d02
Compare
@khenidak @jackfrancis I've added the elb service as a workaround. PTAL |
- port: 8765 | ||
targetPort: 9376 | ||
selector: | ||
app: random |
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.
are you keeping this as random
?
2a9995e
to
9a67497
Compare
docs/clusterdefinition.md
Outdated
@@ -53,6 +53,7 @@ Here are the valid values for the orchestrator types: | |||
| gcLowThreshold | no | Sets the --image-gc-low-threshold value on the kublet configuration. Default is 80. [See kubelet Garbage Collection](https://kubernetes.io/docs/concepts/cluster-administration/kubelet-garbage-collection/) | | |||
| kubeletConfig | no | Configure various runtime configuration for kubelet. See `kubeletConfig` [below](#feat-kubelet-config) | | |||
| kubernetesImageBase | no | Specifies the base URL (everything preceding the actual image filename) of the kubernetes hyperkube image to use for cluster deployment, e.g., `k8s.gcr.io/` | | |||
| loadBalancerSku | no | Sku of Load Balancer and Public IP. Candidate values are: `basic` and `standard`. If not set, it will be default to basic. Requires Kubernetes 1.11 or newer. | |
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.
Please add documentation for LB outbound rule workaround + what is expected in v1.12
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.
missing documentation for the outbound rule workaround
f2806f1
to
ad88ed6
Compare
ad88ed6
to
f508919
Compare
@khenidak @jackfrancis @sozercan doc added, rebased, ILB e2e test on jenkins passed, PTAL |
@@ -67,6 +67,8 @@ | |||
{{end}} | |||
"useManagedIdentityExtension": "{{ UseManagedIdentity }}", | |||
"useInstanceMetadata": "{{ UseInstanceMetadata }}", | |||
"loadBalancerSku": "{{ LoadBalancerSku }}", | |||
"excludeMasterFromStandardLB": "{{ ExcludeMasterFromStandardLB }}", |
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.
you can get rid of this variable assignment...
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.
These two are variables, not parameters.
@@ -132,7 +134,7 @@ | |||
"customSearchDomainsScript": "{{GetKubernetesB64CustomSearchDomainsScript}}", | |||
"sshdConfig": "{{GetB64sshdConfig}}", | |||
{{if not IsOpenShift}} | |||
"provisionScriptParametersCommon": "[concat('ADMINUSER=',parameters('linuxAdminUsername'),' ETCD_DOWNLOAD_URL=',parameters('etcdDownloadURLBase'),' ETCD_VERSION=',parameters('etcdVersion'),' DOCKER_ENGINE_VERSION=',parameters('dockerEngineVersion'),' DOCKER_REPO=',parameters('dockerEngineDownloadRepo'),' TENANT_ID=',variables('tenantID'),' HYPERKUBE_URL=',parameters('kubernetesHyperkubeSpec'),' APISERVER_PUBLIC_KEY=',parameters('apiserverCertificate'),' SUBSCRIPTION_ID=',variables('subscriptionId'),' RESOURCE_GROUP=',variables('resourceGroup'),' LOCATION=',variables('location'),' VM_TYPE=',variables('vmType'),' SUBNET=',variables('subnetName'),' NETWORK_SECURITY_GROUP=',variables('nsgName'),' VIRTUAL_NETWORK=',variables('virtualNetworkName'),' VIRTUAL_NETWORK_RESOURCE_GROUP=',variables('virtualNetworkResourceGroupName'),' ROUTE_TABLE=',variables('routeTableName'),' PRIMARY_AVAILABILITY_SET=',variables('primaryAvailabilitySetName'),' PRIMARY_SCALE_SET=',variables('primaryScaleSetName'),' SERVICE_PRINCIPAL_CLIENT_ID=',variables('servicePrincipalClientId'),' SERVICE_PRINCIPAL_CLIENT_SECRET=',variables('singleQuote'),variables('servicePrincipalClientSecret'),variables('singleQuote'),' KUBELET_PRIVATE_KEY=',parameters('clientPrivateKey'),' TARGET_ENVIRONMENT=',parameters('targetEnvironment'),' NETWORK_PLUGIN=',parameters('networkPlugin'),' VNET_CNI_PLUGINS_URL=',parameters('vnetCniLinuxPluginsURL'),' CNI_PLUGINS_URL=',parameters('cniPluginsURL'),' CLOUDPROVIDER_BACKOFF=',parameters('cloudproviderConfig').cloudProviderBackoff,' CLOUDPROVIDER_BACKOFF_RETRIES=',parameters('cloudproviderConfig').cloudProviderBackoffRetries,' CLOUDPROVIDER_BACKOFF_EXPONENT=',parameters('cloudproviderConfig').cloudProviderBackoffExponent,' CLOUDPROVIDER_BACKOFF_DURATION=',parameters('cloudproviderConfig').cloudProviderBackoffDuration,' CLOUDPROVIDER_BACKOFF_JITTER=',parameters('cloudproviderConfig').cloudProviderBackoffJitter,' CLOUDPROVIDER_RATELIMIT=',parameters('cloudproviderConfig').cloudProviderRatelimit,' CLOUDPROVIDER_RATELIMIT_QPS=',parameters('cloudproviderConfig').cloudProviderRatelimitQPS,' CLOUDPROVIDER_RATELIMIT_BUCKET=',parameters('cloudproviderConfig').cloudProviderRatelimitBucket,' USE_MANAGED_IDENTITY_EXTENSION=',variables('useManagedIdentityExtension'),' USE_INSTANCE_METADATA=',variables('useInstanceMetadata'),' CONTAINER_RUNTIME=',parameters('containerRuntime'),' CONTAINERD_DOWNLOAD_URL_BASE=',parameters('containerdDownloadURLBase'),' POD_INFRA_CONTAINER_SPEC=',parameters('kubernetesPodInfraContainerSpec'),' KMS_PROVIDER_VAULT_NAME=',variables('clusterKeyVaultName'))]", | |||
"provisionScriptParametersCommon": "[concat('ADMINUSER=',parameters('linuxAdminUsername'),' ETCD_DOWNLOAD_URL=',parameters('etcdDownloadURLBase'),' ETCD_VERSION=',parameters('etcdVersion'),' DOCKER_ENGINE_VERSION=',parameters('dockerEngineVersion'),' DOCKER_REPO=',parameters('dockerEngineDownloadRepo'),' TENANT_ID=',variables('tenantID'),' HYPERKUBE_URL=',parameters('kubernetesHyperkubeSpec'),' APISERVER_PUBLIC_KEY=',parameters('apiserverCertificate'),' SUBSCRIPTION_ID=',variables('subscriptionId'),' RESOURCE_GROUP=',variables('resourceGroup'),' LOCATION=',variables('location'),' VM_TYPE=',variables('vmType'),' SUBNET=',variables('subnetName'),' NETWORK_SECURITY_GROUP=',variables('nsgName'),' VIRTUAL_NETWORK=',variables('virtualNetworkName'),' VIRTUAL_NETWORK_RESOURCE_GROUP=',variables('virtualNetworkResourceGroupName'),' ROUTE_TABLE=',variables('routeTableName'),' PRIMARY_AVAILABILITY_SET=',variables('primaryAvailabilitySetName'),' PRIMARY_SCALE_SET=',variables('primaryScaleSetName'),' SERVICE_PRINCIPAL_CLIENT_ID=',variables('servicePrincipalClientId'),' SERVICE_PRINCIPAL_CLIENT_SECRET=',variables('singleQuote'),variables('servicePrincipalClientSecret'),variables('singleQuote'),' KUBELET_PRIVATE_KEY=',parameters('clientPrivateKey'),' TARGET_ENVIRONMENT=',parameters('targetEnvironment'),' NETWORK_PLUGIN=',parameters('networkPlugin'),' VNET_CNI_PLUGINS_URL=',parameters('vnetCniLinuxPluginsURL'),' CNI_PLUGINS_URL=',parameters('cniPluginsURL'),' CLOUDPROVIDER_BACKOFF=',parameters('cloudproviderConfig').cloudProviderBackoff,' CLOUDPROVIDER_BACKOFF_RETRIES=',parameters('cloudproviderConfig').cloudProviderBackoffRetries,' CLOUDPROVIDER_BACKOFF_EXPONENT=',parameters('cloudproviderConfig').cloudProviderBackoffExponent,' CLOUDPROVIDER_BACKOFF_DURATION=',parameters('cloudproviderConfig').cloudProviderBackoffDuration,' CLOUDPROVIDER_BACKOFF_JITTER=',parameters('cloudproviderConfig').cloudProviderBackoffJitter,' CLOUDPROVIDER_RATELIMIT=',parameters('cloudproviderConfig').cloudProviderRatelimit,' CLOUDPROVIDER_RATELIMIT_QPS=',parameters('cloudproviderConfig').cloudProviderRatelimitQPS,' CLOUDPROVIDER_RATELIMIT_BUCKET=',parameters('cloudproviderConfig').cloudProviderRatelimitBucket,' USE_MANAGED_IDENTITY_EXTENSION=',variables('useManagedIdentityExtension'),' USE_INSTANCE_METADATA=',variables('useInstanceMetadata'),' LOAD_BALANCER_SKU=',variables('loadBalancerSku'),' EXCLUDE_MASTER_FROM_STANDARD_LB=',variables('excludeMasterFromStandardLB'),' CONTAINER_RUNTIME=',parameters('containerRuntime'),' CONTAINERD_DOWNLOAD_URL_BASE=',parameters('containerdDownloadURLBase'),' POD_INFRA_CONTAINER_SPEC=',parameters('kubernetesPodInfraContainerSpec'),' KMS_PROVIDER_VAULT_NAME=',variables('clusterKeyVaultName'))]", |
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.
... and change to parameters('excludeMasterFromStandardLB')
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.
Same here, these are variables, not parameters.
@@ -65,6 +65,8 @@ $global:KubeDnsSearchPath = "svc.cluster.local" | |||
|
|||
$global:UseManagedIdentityExtension = "{{WrapAsVariable "useManagedIdentityExtension"}}" | |||
$global:UseInstanceMetadata = "{{WrapAsVariable "useInstanceMetadata"}}" | |||
$global:LoadBalancerSku = "{{WrapAsVariable "loadBalancerSku"}}" | |||
$global:ExcludeMasterFromStandardLB = "{{WrapAsVariable "excludeMasterFromStandardLB"}}" |
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.
... and change to WrapAsParameter "excludeMasterFromStandardLB"
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.
Same here
added a fun comment about superfluous variables and then lgtm! |
/lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: khenidak, sozercan If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@khenidak: changing LGTM is restricted to assignees, and only Azure/acs-engine repo collaborators may be assigned issues. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@ritazh yeah, nevermind, this works fine for now, thanks! |
What this PR does / why we need it:
Adds support for Standard LB for Kubernetes v1.11
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #3468Release note: