Workload resource mapping #1086

sadlerap · 2021-12-23T20:35:22Z

Motivation

We need to be able to be informed where container data lives within a workload resource. This introduces support for workload resource mapping, which is how the specification API defines how we can be informed of this data.

Changes

A lot of the functionality of pipeline.Application has been changed to meet this need. To facilitate the requirement to look up data almost arbitrarily within an workload resource, we need to store and carry around the data to look up where our workload's data lives.

Testing

Unit tests should largely pass (with one exception, see below). The infrastructure for acceptance tests is largely there, but we're still a little short on the actual tests. I'll be working to have some more before this gets merged.

Other notes

This crashes under integration/acceptance test scenarios. This is because go panics whenever I try to do a deep copy of a slice of maps (i.e. []map[string]interface); I haven't quite figured out why this is happing nor how to fix it.
This enables support in both the specification API and our API. Judging from discussion a few weeks ago, we wanted to have a replacement for the bindingPath entry within a service binding; this is probably a more robust solution (current spec issues aside).
Acceptance tests are a little short at the moment. I'm hoping to expand them going forward.

openshift-ci · 2021-12-23T20:35:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2022-01-07T17:04:36Z

Codecov Report

Merging #1086 (c6b8005) into master (78327d1) will decrease coverage by 1.42%.
The diff coverage is 56.08%.

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
- Coverage   58.92%   57.50%   -1.43%     
==========================================
  Files          30       31       +1     
  Lines        1670     2673    +1003     
==========================================
+ Hits          984     1537     +553     
- Misses        560      974     +414     
- Partials      126      162      +36

Impacted Files	Coverage Δ
apis/spec/v1alpha3/servicebinding_webhook.go	`65.38% <ø> (-1.29%)`	⬇️
apis/spec/v1alpha3/zz_generated.deepcopy.go	`12.06% <0.00%> (-7.71%)`	⬇️
pkg/reconcile/pipeline/api.go	`77.77% <ø> (+2.77%)`	⬆️
pkg/reconcile/pipeline/handler/project/impl.go	`51.09% <47.91%> (-11.90%)`	⬇️
pkg/reconcile/pipeline/context/application.go	`64.10% <57.37%> (-13.68%)`	⬇️
pkg/reconcile/pipeline/mapping.go	`64.50% <64.50%> (ø)`
pkg/reconcile/pipeline/context/impl.go	`66.75% <73.19%> (+1.59%)`	⬆️
...kg/reconcile/pipeline/context/spec_binding_impl.go	`68.96% <75.00%> (-3.59%)`	⬇️
pkg/converter/unstructured.go	`72.09% <76.92%> (+49.87%)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78327d1...c6b8005. Read the comment docs.

sadlerap · 2022-01-14T23:13:48Z

This is probably good enough to review, at least from a high level. There are still some parts that might change: some acceptance tests are failing, and I know I need to add some more unit tests to make code coverage happy. However, I doubt the design is going to change all that much from these fixes, and most of this should be in good-enough shape to review.

pedjak

Changes to the userguide should be also a part of this PR.

Side note: I somehow expected less changes on pkg/reconcile/pipeline/handler/project/impl.go

Makefile

config/manager/kustomization.yaml

controllers/common.go

pedjak · 2022-01-15T12:02:37Z

pkg/reconcile/pipeline/builder/pipeline_integration_test.go

+		_, err = client.
+			Resource(v1alpha3.WorkloadResourceMappingGroupVersionResource).
+			Get(c.Background(), serviceGVK.GroupKind().String(), metav1.GetOptions{})
+		Expect(err).To(Equal(errors.NewNotFound(v1alpha3.WorkloadResourceMappingGroupVersionResource.GroupResource(), serviceGVK.GroupKind().String())), "Binding should occur without a workload resource mapping")


why do we need this assertion in the integration scenario?

apis/spec/v1alpha3/cluster_workload_resource_mapping.go

pkg/reconcile/pipeline/context/impl.go

pedjak · 2022-01-17T15:20:00Z

pkg/reconcile/pipeline/context/impl.go

@@ -425,9 +445,31 @@ func (i *impl) Close() error {
 	}
 	for _, app := range i.applications {
 		if app.IsUpdated() {
-			_, err = i.client.Resource(*app.gvr).Namespace(i.bindingMeta.Namespace).Update(context.Background(), app.Resource(), metav1.UpdateOptions{})


why we needed to replace this line?

pkg/reconcile/pipeline/context/application.go

pkg/reconcile/pipeline/handler/project/impl.go

test/acceptance/features/workloadResourceBinding.feature

apis/spec/v1alpha3/cluster_workload_resource_mapping.go

pkg/converter/unstructured.go

baijum · 2022-04-11T03:37:43Z

/retest

baijum · 2022-04-11T05:17:56Z

@baijum - I've added the tests you requested. I wrote them against the spec API; should we also have these tests against the coreos API as well?

I think tests in spec API are enough.

baijum · 2022-04-11T07:56:54Z

/retest

baijum · 2022-04-12T06:19:21Z

/retest

pmacik · 2022-04-12T09:45:12Z

/retest

sadlerap · 2022-04-12T11:25:39Z

/retest

sadlerap · 2022-04-12T13:25:36Z

/retest

sadlerap · 2022-04-12T15:59:19Z

/retest

Implement support for ClusterWorkloadResourceMapping resources and most of their required semantics as dictated by the specification. The following are not implemented, and will be submitted in later patches: - validation on resource creation/modification - correct unbind behavior when the mapping changes - necessary documentation Signed-off-by: Andy Sadler <ansadler@redhat.com>

sadlerap · 2022-04-12T20:48:15Z

/retest

sadlerap · 2022-04-13T15:54:48Z

/retest

sadlerap · 2022-04-13T19:25:38Z

/retest

openshift-ci · 2022-04-13T21:18:33Z

@sadlerap: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.7-acceptance	`6895640`	link	true	`/test 4.7-acceptance`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

baijum · 2022-04-14T05:26:35Z

/retest

baijum · 2022-04-14T11:38:12Z

/lgtm

dperaza4dustbit · 2022-04-19T18:06:31Z

This PR looks good to merge, any improvements we will like to see, lets add a Jira story under epic https://issues.redhat.com/browse/APPSVC-1082, any regression we notice lets add a jira bug.

Implement support for ClusterWorkloadResourceMapping resources and most of their required semantics as dictated by the specification. The following are not implemented, and will be submitted in later patches: - validation on resource creation/modification - correct unbind behavior when the mapping changes - necessary documentation Signed-off-by: Andy Sadler <ansadler@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Dec 23, 2021

openshift-ci bot requested review from jasperchui and pedjak December 23, 2021 20:35

pedjak requested review from baijum and removed request for jasperchui December 23, 2021 21:12

sadlerap force-pushed the workload-resource-mapping branch from 2a673a5 to ce9e9bf Compare January 7, 2022 16:59

sadlerap force-pushed the workload-resource-mapping branch 2 times, most recently from 0cdacdc to 2bbf251 Compare January 10, 2022 13:49

sadlerap force-pushed the workload-resource-mapping branch from 2bbf251 to ea2ef16 Compare January 14, 2022 23:09

sadlerap marked this pull request as ready for review January 14, 2022 23:13

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 14, 2022

pedjak suggested changes Jan 17, 2022

View reviewed changes

sadlerap force-pushed the workload-resource-mapping branch from fefec0d to 0322a92 Compare January 18, 2022 22:30

openshift-ci bot added the needs-rebase label Jan 27, 2022

baijum reviewed Feb 4, 2022

View reviewed changes

test/acceptance/features/workloadResourceBinding.feature Outdated Show resolved Hide resolved

baijum reviewed Feb 4, 2022

View reviewed changes

test/acceptance/features/workloadResourceBinding.feature Outdated Show resolved Hide resolved

baijum reviewed Feb 4, 2022

View reviewed changes

test/acceptance/features/workloadResourceBinding.feature Outdated Show resolved Hide resolved

sadlerap force-pushed the workload-resource-mapping branch from 0322a92 to 1e55fd7 Compare February 9, 2022 21:07

openshift-ci bot removed the needs-rebase label Feb 9, 2022

sadlerap force-pushed the workload-resource-mapping branch from 1e55fd7 to 3caa5df Compare February 9, 2022 21:34

baijum mentioned this pull request Feb 10, 2022

Support Workload Resource Mapping #927

Closed

pmacik linked an issue Feb 10, 2022 that may be closed by this pull request

Support Workload Resource Mapping #927

Closed

sadlerap force-pushed the workload-resource-mapping branch from 3caa5df to 6895640 Compare February 10, 2022 20:19

sadlerap marked this pull request as draft February 21, 2022 13:51

openshift-ci bot added the do-not-merge/work-in-progress label Feb 21, 2022

sadlerap force-pushed the workload-resource-mapping branch 2 times, most recently from 4ab5023 to 2311f33 Compare March 15, 2022 19:59

Kartikey-star reviewed Apr 6, 2022

View reviewed changes

apis/spec/v1alpha3/cluster_workload_resource_mapping.go Outdated Show resolved Hide resolved

Kartikey-star reviewed Apr 6, 2022

View reviewed changes

pkg/converter/unstructured.go Show resolved Hide resolved

sadlerap force-pushed the workload-resource-mapping branch from ee81c5f to f241192 Compare April 8, 2022 20:07

sadlerap force-pushed the workload-resource-mapping branch 4 times, most recently from d628f53 to 3a2b276 Compare April 11, 2022 19:27

sadlerap force-pushed the workload-resource-mapping branch from 3a2b276 to 28dcc77 Compare April 12, 2022 12:49

sadlerap force-pushed the workload-resource-mapping branch from 28dcc77 to f98a9bc Compare April 12, 2022 17:27

sadlerap force-pushed the workload-resource-mapping branch from f98a9bc to c6b8005 Compare April 12, 2022 18:16

openshift-ci bot added the lgtm label Apr 14, 2022

dperaza4dustbit merged commit 6f7dadf into redhat-developer:master Apr 19, 2022

pedjak mentioned this pull request Apr 27, 2022

User Documentation does not describe the usage of WorkloadResourceMapping #1135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workload resource mapping #1086

Workload resource mapping #1086

sadlerap commented Dec 23, 2021

openshift-ci bot commented Dec 23, 2021

codecov bot commented Jan 7, 2022 •

edited

sadlerap commented Jan 14, 2022

pedjak left a comment

pedjak Jan 15, 2022

pedjak Jan 17, 2022

baijum commented Apr 11, 2022

baijum commented Apr 11, 2022

baijum commented Apr 11, 2022

baijum commented Apr 12, 2022

pmacik commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 13, 2022

sadlerap commented Apr 13, 2022

openshift-ci bot commented Apr 13, 2022 •

edited

baijum commented Apr 14, 2022

baijum commented Apr 14, 2022

dperaza4dustbit commented Apr 19, 2022

Workload resource mapping #1086

Workload resource mapping #1086

Conversation

sadlerap commented Dec 23, 2021

Motivation

Changes

Testing

Other notes

openshift-ci bot commented Dec 23, 2021

codecov bot commented Jan 7, 2022 • edited

Codecov Report

sadlerap commented Jan 14, 2022

pedjak left a comment

Choose a reason for hiding this comment

pedjak Jan 15, 2022

Choose a reason for hiding this comment

pedjak Jan 17, 2022

Choose a reason for hiding this comment

baijum commented Apr 11, 2022

baijum commented Apr 11, 2022

baijum commented Apr 11, 2022

baijum commented Apr 12, 2022

pmacik commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 12, 2022

sadlerap commented Apr 13, 2022

sadlerap commented Apr 13, 2022

openshift-ci bot commented Apr 13, 2022 • edited

baijum commented Apr 14, 2022

baijum commented Apr 14, 2022

dperaza4dustbit commented Apr 19, 2022

codecov bot commented Jan 7, 2022 •

edited

openshift-ci bot commented Apr 13, 2022 •

edited