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

Remove docker #2973

Merged
merged 2 commits into from
Jun 25, 2018
Merged

Remove docker #2973

merged 2 commits into from
Jun 25, 2018

Conversation

jessfraz
Copy link
Contributor

closes #2969

This pull request makes it so when the container runtime is set to containerd or clear-containers docker is never installed.

cc @thomastaylor312

Just some clean up after part one in #2967

@ghost ghost assigned jessfraz May 15, 2018
@ghost ghost added the in progress label May 15, 2018
@jessfraz jessfraz force-pushed the remove-docker branch 2 times, most recently from 246358c to 802ab7a Compare May 15, 2018 20:48
@jessfraz
Copy link
Contributor Author

here is clear-containers running with that https://jenkins.azure-containers.io/view/acs-engine/job/k8s-clear-containers/117/console

@ghost ghost assigned jackfrancis May 18, 2018
@codecov
Copy link

codecov bot commented May 18, 2018 •
edited
Loading

Codecov Report

Merging #2973 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2973      +/-   ##
==========================================
- Coverage   53.01%   52.99%   -0.03%     
==========================================
  Files         104      104              
  Lines       15683    15689       +6     
==========================================
  Hits         8314     8314              
- Misses       6627     6633       +6     
  Partials      742      742

@acs-bot acs-bot added the size/L label May 18, 2018
@jackfrancis
Copy link
Member

FYI rebased/force-pushed to accommodate recent changes to CSE in master

jackfrancis
jackfrancis reviewed May 18, 2018
View reviewed changes
parts/k8s/kubernetescustomscript.sh Outdated
@@ -339,8 +339,22 @@ function ensureKubelet() {
}

function extractHyperkube(){
retrycmd_if_failure 100 1 60 docker pull $HYPERKUBE_URL || $ERR_K8S_DOWNLOAD_TIMEOUT
systemctlEnableAndStart hyperkube-extract
TMP_DIR=$(mktemp -d)
Copy link
Member

Choose a reason for hiding this comment

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

@jessfraz is TMP_DIR used anywhere?

jackfrancis
jackfrancis reviewed May 18, 2018
View reviewed changes
test/e2e/runner/cli_provisioner.go Outdated
@@ -242,7 +242,7 @@ func (cli *CLIProvisioner) FetchProvisioningMetrics(path string, cfg *config.Con
agentFiles := []string{"/var/log/azure/cluster-provision.log", "/var/log/cloud-init.log",
"/var/log/cloud-init-output.log", "/var/log/syslog", "/var/log/azure/custom-script/handler.log",
"/opt/m", "/opt/azure/containers/kubelet.sh", "/opt/azure/containers/provision.sh",
"/opt/azure/provision-ps.log", "/var/log/azure/kubelet-status.log", "/var/log/azure/hyperkube-extract-status.log",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the love here!

@jackfrancis
Copy link
Member

@jessfraz img pull command is erroring w/ the following:

creating worker opt failed: cannot find runc binary locally, please install runc

dividinglimits
dividinglimits reviewed May 19, 2018
View reviewed changes
pkg/api/types.go Outdated
@@ -737,6 +737,11 @@ func (a *AgentPoolProfile) HasDisks() bool {
return len(a.DiskSizesGB) > 0
}

// RequiresDocker returns if the kubernetes settings require docker to be installed.
func (a *AgentPoolProfile) RequiresDocker() bool {
return a.KubernetesConfig.ContainerRuntime == "docker"
Copy link

Choose a reason for hiding this comment

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

Please forgive my ignorance, but since these are exported, is it guaranteed that this value will always be lower case during its initialization?

@jessfraz
Copy link
Contributor Author

ah I tested on the one with containerd, will update

@jessfraz
Copy link
Contributor Author

jessfraz commented May 21, 2018 •
edited
Loading

Running:
https://jenkins.azure-containers.io/view/acs-engine/job/k8s-containerd/3/
https://jenkins.azure-containers.io/view/acs-engine/job/k8s-clear-containers/128/
https://jenkins.azure-containers.io/view/acs-engine/job/k8s-network-flannel/7/
https://jenkins.azure-containers.io/view/acs-engine/job/k8s-network-cilium/90

@jessfraz jessfraz force-pushed the remove-docker branch 3 times, most recently from 91fbd17 to 4503665 Compare May 22, 2018 01:02
@jessfraz
Copy link
Contributor Author

looks like dns is failing randomly in the tests is this usual? I am wondering how this might have broken that

@jackfrancis
Copy link
Member

@jessfraz it's testing pod name resolution. What do you see if you build a cluster from this branch and try to resolve DNS from within a pod?

@jessfraz jessfraz force-pushed the remove-docker branch 4 times, most recently from 8a35f22 to ae2a72b Compare June 12, 2018 15:55
@jessfraz
Copy link
Contributor Author

Passed: https://jenkins.azure-containers.io/view/acs-engine/job/k8s-containerd/28/console

@jackfrancis
Copy link
Member

Thanks @jessfraz! @CecileRobertMichon or I can help with the rebase, as the CSE implementation is a sensitive surface area constantly undergoing refinements :)

@jessfraz
Copy link
Contributor Author

jessfraz commented Jun 12, 2018 via email

@jackfrancis
Copy link
Member

FYI rebased, gonna see if we can nudge this into working order

@jackfrancis
Copy link
Member

@jessfraz Just built a v1.8.13 cluster and am able to repro E2E failures. Specifically, resolving external DNS from a pod:

azureuser@k8s-master-25395533-0:~$ kubectl logs validate-dns-hj5br -c validate-bing
Server:    10.0.0.10
Address 1: 10.0.0.10

nslookup: can't resolve 'www.bing.com'
waiting for DNS resolution
nslookup: can't resolve 'www.bing.com'
Server:    10.0.0.10
Address 1: 10.0.0.10

waiting for DNS resolution
azureuser@k8s-master-25395533-0:~$ kubectl logs validate-dns-hj5br -c validate-google
Server:    10.0.0.10
Address 1: 10.0.0.10

waiting for DNS resolution
nslookup: can't resolve 'google.com'
nslookup: can't resolve 'google.com'
Server:    10.0.0.10
Address 1: 10.0.0.10

waiting for DNS resolution

@jessfraz
Copy link
Contributor Author

ah i will spin up and test

@jessfraz jessfraz mentioned this pull request Jun 18, 2018
Merged
@jessfraz jessfraz force-pushed the remove-docker branch 7 times, most recently from 977bf3b to 15f5e1c Compare June 19, 2018 23:19
@jessfraz
Copy link
Contributor Author

it passed \o/

@jessfraz @jackfrancis
cleanup unused e2e configs since these are sent as parameters in jenkins
382bfdd 
Signed-off-by: Jess Frazelle <acidburn@microsoft.com>
@jessfraz
add requiresDocker parameter
4068ddd 
Signed-off-by: Jess Frazelle <acidburn@microsoft.com>
@jackfrancis
Copy link
Member

/lgtm

Thanks @jessfraz!

@acs-bot acs-bot added the lgtm label Jun 25, 2018
@acs-bot
Copy link

acs-bot commented Jun 25, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, jessfraz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jackfrancis jackfrancis merged commit fcbf0b4 into Azure:master Jun 25, 2018
@ghost ghost removed the in progress label Jun 25, 2018
@jessfraz jessfraz deleted the remove-docker branch June 25, 2018 17:55
@UnwashedMeme UnwashedMeme mentioned this pull request Jul 2, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dividinglimits dividinglimits dividinglimits left review comments

@jackfrancis jackfrancis jackfrancis left review comments

Assignees

@jessfraz jessfraz

@jackfrancis jackfrancis

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

don't install docker when using container runtime clear-containers or containerd
4 participants
@jessfraz @jackfrancis @acs-bot @dividinglimits