chore(Dockerfile): best practices

[maxime1907]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Apply best practices recommended by hadolint:

• Lock dependencies for reproducible builds
• Lock image with SHA for reproducible builds
• Mount cache directories for faster builds
• Separate RUN directives to avoid rebuilding every layer each time a python dependency changes
• Put COPY directives at the end to avoid rebuilding every layer each time a playbook file changes

Does this PR introduce a user-facing change?:

Update dockerfile to follow best practices

[k8s-ci-robot]

Hi @maxime1907. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[MrFreezeex]

Hi! Thanks for your contribution. Indeed reproducibility is a good practice and we should aim to have the best reproducibility possible and I am all in team reproducibility in general. Although as you have noticed in your other PR we have failed to update a quite simple list of dependencies so I am not sure adding a bunch more of them is the right way to go.

Also I don't expect for the most of apt deps version to really affects kubespray/ansible in a significant way, so I would not be favorable to pin all of those. Kubespray is designed to run on many kinds of system and I don't think that the majority of our users even use the docker image so it shouldn't matter that much what the version of ssh is on the user's system for instance and pinning every apt deps will just put more strain on the maintainers of kubespray for, IMO, little added value.

Although the rest of the change about caching mounts and moving the RUN around to improve caching looks good indeed 👍

[maxime1907]

Hi! Thanks for your contribution. Indeed reproducibility is a good practice and we should aim to have the best reproducibility possible and I am all in team reproducibility in general. Although as you have noticed in your other PR we have failed to update a quite simple list of dependencies so I am not sure adding a bunch more of them is the right way to go.

Also I don't expect for the most of apt deps version to really affects kubespray/ansible in a significant way, so I would not be favorable to pin all of those. Kubespray is designed to run on many kinds of system and I don't think that the majority of our users even use the docker image so it shouldn't matter that much what the version of ssh is on the user's system for instance and pinning every apt deps will just put more strain on the maintainers of kubespray for, IMO, little added value.

Although the rest of the change about caching mounts and moving the RUN around to improve caching looks good indeed 👍

Hi! Yes i understand your point of view so i have removed the pinned apt dependencies so should be good to go!

[MrFreezeex]

Thanks!

One small comment about the dockerfile syntax pinning but outside of that it looks very good thanks! Although could you apply the same set of changes to pipeline.Dockerfile which is used intensively in the CI?

[maxime1907]

Thanks!

One small comment about the dockerfile syntax pinning but outside of that it looks very good thanks! Although could you apply the same set of changes to pipeline.Dockerfile which is used intensively in the CI?

I will open another PR when these are merged:

• chore(Dockerfile): python requirements file #10700
• chore(Dockerfile): best practices #10708

[MrFreezeex]

Arf sorry for that the pipeline seems to be currently too busy (https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/pipelines)... Could you amend/push force in like ~2hours (or more).

Asides from that thank you very much for this contribution!
/lgtm

[MrFreezeex]

/ok-to-test

[maxime1907]

Arf sorry for that the pipeline seems to be currently too busy (https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/pipelines)... Could you amend/push force in like ~2hours (or more).

Asides from that thank you very much for this contribution! /lgtm

Oh i see but i think you can just close and reopen my PR and this will retrigger github pipelines!

[MrFreezeex]

Arf sorry for that the pipeline seems to be currently too busy (https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/pipelines)... Could you amend/push force in like ~2hours (or more).
Asides from that thank you very much for this contribution! /lgtm

Oh i see but i think you can just close and reopen my PR and this will retrigger github pipelines!

I can't do that unfortunately :(

[MrFreezeex]

/lgtm

[floryut]

@maxime1907 Thank you 👍

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, maxime1907, MrFreezeex

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:

• OWNERS [floryut]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment