Skip to content

Build and verify admission controller for vstatefulset #610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 18 commits into from
Closed

Build and verify admission controller for vstatefulset #610

wants to merge 18 commits into from

Conversation

hank95179
Copy link
Contributor

@hank95179 hank95179 commented Apr 22, 2025 •
edited by marshtompsxd
Loading

This PR defines the state_validation spec function for a StatefulSet object.
Rules include the following fields. The fields selector, template, and replicas are omitted below since they are identical to ReplicaSet and Deployment

  • serviceName (string): Optional. Can also be empty.
  • updateStrategy (StatefulSetUpdateStrategy): Optional
    • updateStrategy.type (string): Optional. Should be either "RollingUpdate" or "OnDelete"
    • updateStrategy.rollingUpdate (RollingUpdateStatefulSetStrategy): Optional. Should only be present when type is "RollingUpdate"
      • updateStrategy.rollingUpdate.maxUnavailable (IntOrString): Optional. Assumes unmarshalled to a number for now, must be > 0
      • updateStrategy.rollingUpdate.partition (int32): Optional. Must be >= 0
  • podManagementPolicy (string): Optional. Either "OrderedReady" or "Parallel"
  • revisionHistoryLimit (int32): Optional. Somehow no restrictions
  • volumeClaimTemplates ([]PersistentVolumeClaim): Optional. Assume state_validation is implemented for PersistentVolumeClaim for now.
  • minReadySeconds (int32): Optional. Should be >= 0
  • persistentVolumeClaimRetentionPolicy (StatefulSetPersistentVolumeClaimRetentionPolicy): Optional
    • whenDeleted, whenScaled (string): Both either "Retain" or "Delete"
  • ordinals.start (int32): Optional. Should be >= 0

hank95179 and others added 2 commits April 22, 2025 14:39
@hank95179
upload spec prototype
2171148
@fredred375
fix: spec and comment out for ci to work (#4)
db9eb49 
Added/fix some specs for statefulset to make complete
These specs are also commented out so that it passes ci
@fredred375
Copy link
Contributor

fredred375 commented Apr 22, 2025 •
edited
Loading

@hank95179 I think we are still missing specs for:

DeploymentSet

  • strategy (DeploymentStrategy)
  • revisionHistoryLimit (int32)
  • progressDeadlineSeconds (int32)

StatefulSet

  • updateStrategy (StatefulSetUpdateStrategy)

fredred375
fredred375 reviewed Apr 23, 2025
View reviewed changes
src/v2/controllers/vstatefulset_controller/trusted/spec_type.rs Outdated


// updateStrategy.rollingUpdate.partition is non-negative
&&& self.spec.update_strategy.get_Some_0().rolling_update.get_Some_0().partition.is_Some() ==>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agrama64 I think we should still check for rolling_update.is_Some() before using get_Some_0()

@marshtompsxd
Copy link
Collaborator

@hank95179 @fredred375 could you submit two PRs, one for stateful set and one for deployment set? You can keep this PR for stateful set. Also, could you change the first comment to describe what this PR does?

marshtompsxd
marshtompsxd reviewed Apr 25, 2025
View reviewed changes
src/v2/controllers/vstatefulset_controller/trusted/spec_types.rs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the code commented out now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented out the new specs for both deployment and statefulset so that it doesnt mess up ci. Primarily for deployment, but also commented out statefulset just to be safe.
But yes, that probably isnt needed for statefulset and everything should still work

@fredred375
revert: changes for deployment (#11)
92aa0dc
@fredred375
chore: add comment back (#12)
cebcc50
@fredred375
fix: statefulset state_validation function (#13)
f0b1109 
Found various mistakes in the proposed spec:
1. serviceName is not required and can even be empty
2. Missing ")"
3. revisionHistoryLimit can be any integer
4. Minor fix and comments on volume_claim_templates
5. is_some get_some logic for persistentVolumeClaim... and ordinals.start
@hank95179
Copy link
Contributor Author

@marshtompsxd I have create another PR for deployment and this one PR will only for statefulset.

@hank95179 hank95179 changed the title upload spec prototype Separate Statefulset spec Apr 26, 2025
@marshtompsxd
Copy link
Collaborator

@hank95179 Can you uncomment the code and see whether Verus reports any error?

@marshtompsxd
Copy link
Collaborator

There should be an entry file just like https://github.com/anvil-verifier/anvil/blob/main/src/v2_vdeployment_controller.rs that imports the statefulset files simply to check whether everything compiles and passes Verus' check

@fredred375
add: StatefulSet spec_types and ci (#15)
32a3680 
* add: temporary statefulsetview definitions for verfication

* add: ci for statefulset verification
@fredred375
Copy link
Contributor

@marshtompsxd
Added:

  1. Entrypoint v2_vstatefulset_controller.rs
  2. Copy pasted code from VReplicaSet spec types to populate VStatefulSet spec types for verus to understand state_validation
  3. CI job to verify the above entrypoint and spec types

@marshtompsxd
Copy link
Collaborator

@hank95179 could you address this conflict with ci.yml? And after that, can you continue working on this branch to develop the admission controller for vstatefulset?

@marshtompsxd marshtompsxd changed the title Separate Statefulset spec Build and verify admission controller for vstatefulset Apr 30, 2025
@marshtompsxd
Copy link
Collaborator

Should we close this PR?

@fredred375
Copy link
Contributor

@hank95179 we can go on and close this

@hank95179 hank95179 closed this May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marshtompsxd marshtompsxd marshtompsxd left review comments

@fredred375 fredred375 fredred375 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hank95179 @fredred375 @marshtompsxd @agrama64