Volume resource requirements

[pohly]

What type of PR is this?

/kind cleanup
/kind api-change
/sig storage

What this PR does / why we need it:

PVC and containers shared the same ResourceRequirements struct to define their
API. When resource claims were added, that struct got extended, which
accidentally also changed the PVC API. To avoid such a mistake from happening
again, PVC now uses its own VolumeResourceRequirements struct.

Which issue(s) this PR fixes:

Fixes #115845

Special notes for your reviewer:

The struct has to have a Claims field because someone might have added it to
some YAML file for use with Kubernetes 1.28 even though it didn't do
anything. Removing the field in Kubernetes 1.29 would cause OpenAPI-based
validation in kubectl to reject such a YAML file.

Does this PR introduce a user-facing change?

Go API: the ResourceRequirements struct needs to be replaced with VolumeResourceRequirements for use with volumes.

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[SataQiu]

Mark it as deprecated? And throw a warning when user set it?

[pohly]

Mark as deprecated how? A free-form comment?

Do Go linters recognize // Deprecated: for fields? That's worth checking.

For the warning, that would be a server-side warning, right? I know those exist, but don't know how they are implemented. Do you have a pointer for me?

[pohly]

Do Go linters recognize // Deprecated: for fields?

No, staticcheck only detects usage of deprecated structs. It still makes sense to use the Deprecated comment because it makes the intent clearer.

For the warning, that would be a server-side warning, right?

https://kubernetes.io/blog/2020/09/03/warnings/#future-possibilities mentions "warnings about fields" as a future extension. I've asked on #sig-api-machinery whether it is possible today.

[SataQiu]

@pohly a server-side warning example

kubernetes/pkg/registry/core/persistentvolume/strategy.go

Lines 79 to 82 in c984d53

	// WarningsOnCreate returns warnings for the creation of the given object.
	func (persistentvolumeStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string {
	return pvutil.GetWarningsForPersistentVolume(obj.(*api.PersistentVolume))
	}

[pohly]

That was already wired up for PersistentVolumeClaimSpec, so all I had to do was add a new if check there - done.

[liggitt]

I actually think we should drop the field (with an appropriate tombstone comment recording the json field name and protobuf tag used)

[pohly]

"Drop the field" as in "not have it in the struct at all" (except for the comments)?

So breaking client-side validation (= YAML file has it, kubectl complains about unknown field) would be acceptable?

If we can do that, then let's remove the field. It would be cleaner. I just didn't dare... 😅

[liggitt]

it was added in 1.27 documented as alpha, right? I'd drop the field in 1.28 and backport a change to 1.27 to clear the field on write from PVCs as well

[pohly]

Clearing on write was already implemented in 1.27: #115928.

Sounds like dropping the field is acceptable. I'll change this PR accordingly.

[pohly]

New version pushed which no longer has the field, only a comment about it in core/v1/types.go.

[jiahuif]

/label api-review
/assign @SataQiu
Thank you!
/triage accepted

[liggitt]

needs rebase/regen now that the 1.28.0 API fixtures have been captured and 1.26.0 fixtures dropped

[pohly]

I had already rebased? Should be okay right now.

[pohly]

Ah, I see - there was no merge conflict anymore, but staging/src/k8s.io/api/testdata had to be refreshed nonetheless to cover 1.28.0 - done.

[liggitt]

drop all the v1.26.0 files

[pohly]

Done.

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b7eda65dfb9fbd07d1c874788a80f6578f0dfd83

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pohly

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:

• api/OWNERS [liggitt]
• pkg/api/OWNERS [liggitt]
• pkg/apis/OWNERS [liggitt]
• pkg/controller/history/OWNERS [liggitt]
• pkg/controller/statefulset/OWNERS [liggitt]
• pkg/controller/volume/OWNERS [liggitt]
• pkg/generated/openapi/OWNERS [liggitt]
• pkg/kubelet/volumemanager/OWNERS [liggitt]
• pkg/quota/v1/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]
• pkg/scheduler/OWNERS [liggitt]
• pkg/volume/OWNERS [liggitt]
• plugin/pkg/admission/OWNERS [liggitt]
• staging/src/k8s.io/api/OWNERS [liggitt]
• staging/src/k8s.io/client-go/applyconfigurations/OWNERS [liggitt]
• staging/src/k8s.io/component-helpers/storage/OWNERS [liggitt]
• test/OWNERS [liggitt,pohly]

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

[pohly]

/retest

[neolit123]

this is a breaking change for consumers of the Go struct.
while we care more to not break the serializable API, the change should have had an action-required in the release note.

[pohly]

Do we have documentation on when to use "action-required"? Would it have helped you in this case?

I keep hearing all kinds of opinions about whether Go API changes need to be mentioned at all. I added a release note, but not the "action required" because I assumed that this is reserved for changes that otherwise might silently break users on upgrades. This PR is breaking compilation in an obvious way, so developers will notice that they need to take some action.

[liggitt]

I think a release note is sufficient. People who happen to be using this alpha field in their go structs (who I expect will be vanishingly small) will know they need to take action when they update and their code needs a change before it will compile.

I think action required should be reserved for changes which will surprise or break people if they don't read the release notes, or which they can prepare their cluster or workloads for in advance. This is not such a change.

[neolit123]

FWIW, the label is documented as "Denotes a PR that introduces potentially breaking changes that require user action.".
as far as my interpretation goes, it should be added to breaking changes of any kind that require user action.

i was pinged downstream about this, not on a urgent note, and while a go build will catch the problem of the go struct that did not change for 7 years, similar would apply to a client trying to reach a REST API that changed, the user can find right away.

might be worthwhile for us to document some of these aspects in API docs.

[liggitt]

it should be added to breaking changes of any kind that require user action

If we expand that section of the release notes to include every signature change of every go function in a staging library, that section of the release notes will be so noisy it will be useless. I think it should be reserved for things which require configuration or API changes, would cause problems if they were missed, and could be missed in a cluster upgrade. This is not a change like that.

Upgrading a cluster to the release that included this change does not require people to update their client-go libraries, so I really don't think action is required here.

while a go build will catch the problem of the go struct that did not change for 7 years

to be fair, this was a brand new alpha field, not one that had existed for years, so I sincerely doubt anyone was using it in a way that would require action on a cluster upgrade

edit: my mistake, I mixed this up with a different fixup to a recent alpha field. This field was not alpha.

[pohly]

As much as I'd like to believe that the impact was low, unfortunately that's not the case here: this is a change in PersistentVolumeClaimSpec, which is a GA API. I can understand that consumers of our Go API are upset because this causes them work, but I still think it was the right thing to do because accidentally changing the Kubernetes API by sharing the same struct is worse.

[neolit123]

on my end it was more of a short discussion in the lines of - "is it OK to change the Go structs of a GA API like that?". i responded that for k8s and some other REST APIs projects out there it's not considered an issue. EOD, it depends on what rules a project has.