kubectl explain for ContainerSource

[aavarghese]

Proposed Changes

Add content in CRDs to generate kubectl explain content for ContainerSource ONLY
🐛 partial fix for bug #3054

[knative-prow-robot]

Hi @aavarghese. Thanks for your PR.

I'm waiting for a knative 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.

[aavarghese]

/cc @lionelvillard

[lionelvillard]

/ok-to-test

[lionelvillard]

/test pull-knative-eventing-conformance-tests

[lionelvillard]

The YAML is not valid: error processing import paths in "/tmp/ci-2020-08-17-21-45-11-ooJdE8ALcn/config/300-containersource.yaml": yaml: line 323: did not find expected key

@aavarghese

[lionelvillard]

Now the CRD is too big: The CustomResourceDefinition "containersources.sources.knative.dev" is invalid: metadata.annotations: Too long: must have at most 262144 characters

@nachocano @nlopezgi @aavarghese I suggest to just drop the PodSpec schema information. WDYT?

[nlopezgi]

One of the things I noted from the generator is that it is creating one more identation than necessary. Note spec is two levels down from properties in line 38 and ceOverrides is down two from properties in line 41. This extra identation will add up and might result in the file having more characters than necessary?

[nlopezgi]

Now the CRD is too big: The CustomResourceDefinition "containersources.sources.knative.dev" is invalid: metadata.annotations: Too long: must have at most 262144 characters

@nachocano @nlopezgi @aavarghese I suggest to just drop the PodSpec schema information. WDYT?

It might be worth to fix the identation, there seems to be lots of lines that are very short due to being over-indented. See my comment in the PR above. Might be worth trying to fix before loosing content.

[nlopezgi]

This block is repeated identically 2 times maybe? you can use placeholders to avoid duplication as I did in #3869 (see https://github.com/knative/eventing/pull/3869/files#diff-360926796c72432f2956878c2251f3d8R221). You can probably look for other duplicate blocks

[aavarghese]

On further analysis, looks like that block isn't duplicated entirely the same. There are a few words that have been changed between blocks - like "affinity" vs "non-affinity"....so unfortunately, I can't use a placeholder in this example. Looking for more dup blocks....

[nlopezgi]

Now the CRD is too big: The CustomResourceDefinition "containersources.sources.knative.dev" is invalid: metadata.annotations: Too long: must have at most 262144 characters

@nachocano @nlopezgi @aavarghese I suggest to just drop the PodSpec schema information. WDYT?

There's also duplication that can be avoided via use of placeholders, see how I did it in https://github.com/knative/eventing/pull/3869/files#diff-360926796c72432f2956878c2251f3d8R221

[aavarghese]

Great. Thanks for the suggestions. I won't be able to make another pass through this very soon since I'm taking days off this week. I will try to get another version out for testing asap...

[aavarghese]

After many hours of making manual changes to this giant yaml (fixing indentations, joining multiple lines etc), I was unable to get it to a size that meets the requirement. Just too BIG. And there aren't many blocks that are exactly the same, so I wasn't able to leverage placeholders. I used Visual Studio's Check Duplicate lines plugin to help me here...

At the end, as per @lionelvillard's suggestion, I modified the PodSpec description to see kubectl explain pod.spec and removed the properties for pod.spec from container source's yaml (since kubectl explain pod.spec shows the same info)

[lionelvillard]

/test pull-knative-eventing-integration-tests

[aavarghese]

I see this error in integration tests:

creation.go:510: Failed to create containersource "e2e-container-source": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: missing field(s): spec.containers

creation.go:515: Failed to create containersource "e2e-container-source": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: missing field(s): spec.containers

[lionelvillard]

@nachocano can you take a look at the errors above? Maybe the e2e test is wrong?

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-conformance-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-conformance-tests:

test/conformance.TestBrokerV1Beta1DataPlaneConsumer/Replies_are_accepted_and_delivered
test/conformance.TestBrokerV1Beta1DataPlaneConsumer

[nlopezgi]

I see this error in integration tests:

creation.go:510: Failed to create containersource "e2e-container-source": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: missing field(s): spec.containers

creation.go:515: Failed to create containersource "e2e-container-source": admission webhook "validation.webhook.eventing.knative.dev" denied the request: validation failed: missing field(s): spec.containers

Not sure if its helpful for your error, but in my PRs I found I had to remove the required: blocks as, it seems, some tests are not creating the status with all the required fields

[lionelvillard]

/test pull-knative-eventing-conformance-tests

[nachocano]

My apologies, I just saw this. I'm being bombarded with emails, I think I have around 200 not read...
Briefly looked at the logs, seems that it might not be a container source problem. I saw some broker ones were failing...
Let's see what happens with the retest... If it still fails, I'll dig in in a couple of hours...

Thanks for doing this @aavarghese and thanks @lionelvillard and @nlopezgi for reviewing!

[nachocano]

"validation.webhook.eventing.knative.dev" denied the request: validation failed: missing field(s): spec.containers

I didn't see that log. Maybe was from a previous run before removing the podSpec from the yaml?
Let's see if this run succeeds or I'll dig deeper...

BTW, I manually applied this changed CRD and created a ContainerSource and it worked...

[lionelvillard]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aavarghese, lionelvillard

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 [lionelvillard]

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

[nlopezgi]

/lgtm