Moving Build Dockerfile to be defined in the ConfigMap #160

yevgeny-shnaidman · 2022-11-22T12:45:51Z

Currently dockerfile is defined a multiline string in the Build field of the KernelMapping. This PR does the following: 1) dockerfile is now defined in a dedicated configmap (created by customer)
with the key being: dockerfile
2) Build struct now contains reference to the ConfigMap, instead of
a string
3) change the flow of e2e workflow for the in-cluster build
to use the new configuration

k8s-ci-robot · 2022-11-22T12:45:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yevgeny-shnaidman

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~~ [yevgeny-shnaidman]

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

yevgeny-shnaidman · 2022-11-22T12:46:30Z

/assign @qbarrand
/assign @ybettan
/assign @mresvanis

k8s-ci-robot · 2022-11-22T12:46:33Z

@yevgeny-shnaidman: GitHub didn't allow me to assign the following users: mresvanis.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @qbarrand
/assign @ybettan
/assign @mresvanis

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.

yevgeny-shnaidman · 2022-11-22T12:47:21Z

/cc @mresvanis

k8s-ci-robot · 2022-11-22T12:47:23Z

@yevgeny-shnaidman: GitHub didn't allow me to request PR reviews from the following users: mresvanis.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mresvanis

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.

codecov-commenter · 2022-11-22T12:49:57Z

Codecov Report

Base: 71.86% // Head: 71.60% // Decreases project coverage by -0.26% ⚠️

Coverage data is based on head (3fe00c9) compared to base (351def7).
Patch coverage: 44.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   71.86%   71.60%   -0.27%     
==========================================
  Files          25       25              
  Lines        2417     2437      +20     
==========================================
+ Hits         1737     1745       +8     
- Misses        597      605       +8     
- Partials       83       87       +4

Impacted Files	Coverage Δ
controllers/module_reconciler.go	`67.69% <ø> (ø)`
internal/build/helper.go	`0.00% <0.00%> (ø)`
main.go	`3.87% <0.00%> (ø)`
internal/build/job/maker.go	`87.30% <47.82%> (-5.60%)`	⬇️
internal/build/job/manager.go	`78.46% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

qbarrand · 2022-11-22T13:37:27Z

Fixes #115

ybettan · 2022-11-22T13:37:37Z

api/v1beta1/module_types.go

@@ -60,7 +60,8 @@ type Build struct {
 	// BuildArgs is an array of build variables that are provided to the image building backend.
 	BuildArgs []BuildArg `json:"buildArgs"`

-	Dockerfile string `json:"dockerfile"`
+	// ConfigMap that holds Dockerfile contents
+	DockerfileRef *v1.LocalObjectReference `json:"dockerfileRef"`


I think this will cause an issue:
Change to the config map won't trigger a reconcile.

@yevgeny-shnaidman can we leave Dockerfile as an option? It has the benefit of not breaking the API, which requires a version bump and a conversion webhook...

we don't want to be triggered by ConfigMap. If the image is already build, then we won't run the Build flow again, and if the build has failed, then the reconcile will be triggered by us

@qbarrand i think that it cause our code to become confusing: we will need to support both option, decide which one to take, which one to replace, we will also have the same issue in kaniko configuration. Since we have not released the official API yet, maybe it is better to change it and not to support all the options just in order to avoid conversion hooks?

WDYT about having both fields and failing the reconciliation? When we implement the validating webhook, we can add that check.

I don't see the reason to support both options, it is just confusing, both to us and to the customer. Anyway you will have to support both options in the code, which adds to the confusion. Why are you worried about conversion webhook? I think that at this stage we can just tell customers to move to new version and change their code, i think that any customer we have right now is at very starting stage

If we are already breaking the API. Do you have any objection to have that one as well without a conversion webhook?

Are you good with that, @qbarrand ?

OK to remove the field per @bthurber's decision.
How about changing the new field name to DockerfileConfigMap?

If we are already breaking the API then let's merge this one as well #86.

qbarrand · 2022-11-22T13:43:21Z

api/v1beta1/module_types.go

@@ -60,7 +60,8 @@ type Build struct {
 	// BuildArgs is an array of build variables that are provided to the image building backend.
 	BuildArgs []BuildArg `json:"buildArgs"`

-	Dockerfile string `json:"dockerfile"`
+	// ConfigMap that holds Dockerfile contents
+	DockerfileRef *v1.LocalObjectReference `json:"dockerfileRef"`


@yevgeny-shnaidman can we leave Dockerfile as an option? It has the benefit of not breaking the API, which requires a version bump and a conversion webhook...

controllers/module_reconciler.go

qbarrand · 2022-11-22T13:46:19Z

internal/build/job/maker.go

+	dockefileData, err := m.getDockerfileContents(ctx, buildConfig, mod.Namespace)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get dockerfile contents: %v", err)
+	}
 	specTemplate := v1.PodTemplateSpec{
 		ObjectMeta: metav1.ObjectMeta{
-			Annotations: map[string]string{"Dockerfile": buildConfig.Dockerfile},
+			Annotations: map[string]string{"Dockerfile": dockefileData},


I would rather mount the ConfigMap into the pod.

i will check if kaniko supports that option

@yevgeny-shnaidman I believe we only need to change the volume there.

Fixed. Please review the changes:

new Volume definition\

new hash calculation

changed build config pickup implementation

ci/dockerfile-configmap.yaml

internal/build/job/maker.go

config/manager/kustomization.yaml

internal/build/helper.go

internal/build/job/maker.go

Currently dockerfile is defined a multiline string in the Build field of the KernelMapping. This PR does the following: 1) dockerfile is now defined in a dedicated configmap (created by customer) with the key being: dockerfile 2) Build struct now contains reference to the ConfigMap, instead of a string 3) change the flow of e2e workflow for the in-cluster build to use the new configuration

qbarrand · 2022-11-24T08:18:12Z

/lgtm

…rnetes-sigs#160) This change renames the `config/manifests/bases/kmm-operator.clusterserviceversion.yaml` file to `kernel-module-management.clusterserviceversion.yaml`, in order to fix `make bundle`. The file name must match the `projectName` defined in the `PROJECT` file, or `operator generate kustomize manifests -q` will not find it and ask all the questions to generate it. This change also update some tools to their latest versions: - update golangci-lint - update controller-gen - update kustomize - update OPM, to include file based catalog creation https://olm.operatorframework.io/docs/reference/file-based-catalogs/#design Signed-off-by: Fabien Dupont <fdupont@redhat.com> Signed-off-by: Fabien Dupont <fdupont@redhat.com> Upstream-Commit: 25abfb1 Co-authored-by: Fabien Dupont <fdupont@redhat.com>

k8s-ci-robot requested review from qbarrand and ybettan November 22, 2022 12:45

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2022

k8s-ci-robot assigned qbarrand and ybettan Nov 22, 2022

yevgeny-shnaidman force-pushed the yevgeny/ISSUE-115 branch from f674e51 to 3fe00c9 Compare November 22, 2022 13:19

ybettan reviewed Nov 22, 2022

View reviewed changes

qbarrand reviewed Nov 22, 2022

View reviewed changes

ybettan reviewed Nov 22, 2022

View reviewed changes

ci/dockerfile-configmap.yaml Outdated Show resolved Hide resolved

ybettan reviewed Nov 22, 2022

View reviewed changes

internal/build/job/maker.go Outdated Show resolved Hide resolved

yevgeny-shnaidman force-pushed the yevgeny/ISSUE-115 branch 7 times, most recently from b671cd8 to 7bc6abf Compare November 23, 2022 14:12

qbarrand reviewed Nov 23, 2022

View reviewed changes

yevgeny-shnaidman force-pushed the yevgeny/ISSUE-115 branch from 7bc6abf to 6adc5e1 Compare November 24, 2022 08:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 24, 2022

k8s-ci-robot merged commit a3d7e28 into kubernetes-sigs:main Nov 24, 2022

qbarrand mentioned this pull request Feb 7, 2023

decouple building process from Module CRD/CR #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving Build Dockerfile to be defined in the ConfigMap #160

Moving Build Dockerfile to be defined in the ConfigMap #160

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

codecov-commenter commented Nov 22, 2022 •

edited

qbarrand commented Nov 22, 2022

ybettan Nov 22, 2022

qbarrand Nov 22, 2022

yevgeny-shnaidman Nov 22, 2022

yevgeny-shnaidman Nov 22, 2022

qbarrand Nov 22, 2022

yevgeny-shnaidman Nov 22, 2022

ybettan Nov 23, 2022

yevgeny-shnaidman Nov 23, 2022 •

edited

qbarrand Nov 23, 2022

ybettan Nov 23, 2022

qbarrand Nov 22, 2022

qbarrand Nov 22, 2022

yevgeny-shnaidman Nov 22, 2022

ybettan Nov 22, 2022

qbarrand Nov 22, 2022

yevgeny-shnaidman Nov 23, 2022

qbarrand commented Nov 24, 2022

Moving Build Dockerfile to be defined in the ConfigMap #160

Moving Build Dockerfile to be defined in the ConfigMap #160

Conversation

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

yevgeny-shnaidman commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

codecov-commenter commented Nov 22, 2022 • edited

Codecov Report

qbarrand commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yevgeny-shnaidman Nov 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbarrand commented Nov 24, 2022

codecov-commenter commented Nov 22, 2022 •

edited

yevgeny-shnaidman Nov 23, 2022 •

edited