Skip to content
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

Override default pod spec with StrategicMergePatch #321

Merged
merged 11 commits into from
Oct 4, 2019
Merged

Override default pod spec with StrategicMergePatch #321

merged 11 commits into from
Oct 4, 2019

Conversation

PrasadG193
Copy link
Contributor

@PrasadG193 PrasadG193 commented Sep 26, 2019 •
edited by SupriyaKasten

Change Overview

Pull request type

Please check the type of change your PR introduces:

  • Work in Progress
  • Refactoring (no functional changes, no api changes)
  • Trivial/Minor
  • Bugfix
  • Feature
  • Documentation

Issues

  • #XXX

Test Plan

  • Manual
  • Unit test
  • E2E

SupriyaKasten
SupriyaKasten reviewed Sep 30, 2019
View reviewed changes
pkg/kube/pod.go Outdated Show resolved Hide resolved
@PrasadG193 PrasadG193 marked this pull request as ready for review September 30, 2019 13:04
SupriyaKasten
SupriyaKasten reviewed Oct 3, 2019
View reviewed changes
docs/templates.rst
@@ -24,6 +24,7 @@ The TemplateParam struct is defined as:
Options map[string]string
Object map[string]interface{}
Phases map[string]*Phase
PodOverride map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PodOverride map[string]interface{}
PodOverride strategicpatch.JSONMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that will confuse reader which format to use while passing the parameter from the Actionset. That's why I've kept it's type as map[string]interface{} in functions page also
https://github.com/kanisterio/kanister/pull/321/files#diff-610c3344a79db2ab08c4ef6b15842adeR416
Let me know that you think

SupriyaKasten
SupriyaKasten reviewed Oct 3, 2019
View reviewed changes
pkg/testing/e2e_test.go Outdated
}
sec, err = s.cli.CoreV1().Secrets(s.namespace).Create(sec)
c.Assert(err, IsNil)
p := &crv1alpha1.Profile{
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrasadG193 can we make use of func NewTestProfile(namespace string, secretName string) from pkg/testutil/testutil.go here ?

SupriyaKasten
SupriyaKasten reviewed Oct 3, 2019
View reviewed changes
pkg/apis/cr/v1alpha1/types.go
// default pod specs
PodOverride v1.PodSpec `json:"podOverride,omitempty"`
PodOverride sp.JSONMap `json:"podOverride,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrasadG193 please add these file changes along with pkg/param/param.go into a separate PR!

SupriyaKasten
SupriyaKasten reviewed Oct 3, 2019
View reviewed changes
pkg/kube/pod.go Outdated
@@ -133,3 +144,58 @@ func WaitForPodCompletion(ctx context.Context, cli kubernetes.Interface, namespa
})
return errors.Wrap(err, "Pod did not transition into complete state")
}

// PatchDefaultPodSpecs use Strategic Merge to patch default pod specs with the passed specs
func PatchDefaultPodSpecs(defaultPodSpecs v1.PodSpec, override sp.JSONMap) (v1.PodSpec, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(From meeting discussion) Lets avoid exporting func PatchDefaultPodSpecs and CreateAndMergeJsonPatch

Copy link

Choose a reason for hiding this comment

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

We'll still need CreateAndMergeJsonPatch I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, CreateAndMergeJsonPatch is being used in function package

vkamra
vkamra reviewed Oct 4, 2019
View reviewed changes
pkg/function/copy_volume_data.go Outdated
@@ -133,11 +136,24 @@ func (*copyVolumeDataFunc) Exec(ctx context.Context, tp param.TemplateParams, ar
if err = OptArg(args, CopyVolumeDataEncryptionKeyArg, &encryptionKey, restic.GeneratePassword()); err != nil {
return nil, err
}
if err = OptArg(args, CopyVolumeDataPodOverrideArg, &podOverride, tp.PodOverride); err != nil {
Copy link

Choose a reason for hiding this comment

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

You might want to move this all into a helper function in the function pkg - possibly in args.go. It'll simplify the repeated code in the functions.

e.g.

// GetPodSpecOverride 
func GetPodSpecOverride(tp param.TemplateParams, args map[string]interface{}, argName string) (podOverride map[string]interface{}, err error)

vkamra
vkamra reviewed Oct 4, 2019
View reviewed changes
pkg/kube/pod.go
}

// CreatePod creates a pod with a single container based on the specified image
func CreatePod(ctx context.Context, cli kubernetes.Interface, opts *PodOptions) (*v1.Pod, error) {
volumeMounts, podVolumes := createVolumeSpecs(opts.Volumes)
defaultSpecs := v1.PodSpec{
Copy link

Choose a reason for hiding this comment

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

Minor - just call this podSpecs given that it is no longer "default" after line 66

vkamra
vkamra reviewed Oct 4, 2019
View reviewed changes
Copy link

@vkamra vkamra left a comment

Choose a reason for hiding this comment

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

LGTM - @SupriyaKasten - will let you approve

@PrasadG193
Use StrategicMergePatch to patch default pod spec
b3cd140 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Add support to override default Pod Specs in KubeTask function
e2aa317 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Add support to override pod specs in all Kanister functions
a917ebd 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Add unit tests for merge patch functions
c500d01 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Use strategicpatch.JSONMap type for PodOverride specs
8be0093 
instead of map[string]interface{}

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Refactoring - move common code to a function
6a26155 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
UT: Add test for Blueprint spec override with Actionset
2b8afab 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
Update docs for PodOverride support
2edf082 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193
e2e_test: Add e2e test for KubeTask function
398782e 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193 PrasadG193 force-pushed the pod-override branch 2 times, most recently from e1d7dac to 665baf0 Compare October 4, 2019 10:33
@PrasadG193
Addressed review comments
2a741c4 
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@SupriyaKasten
Copy link
Contributor

🚀

@mergify mergify bot merged commit 7124ce1 into kanisterio:master Oct 4, 2019
@vkamra vkamra mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vkamra vkamra vkamra left review comments

@SupriyaKasten SupriyaKasten SupriyaKasten approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@PrasadG193 @SupriyaKasten @vkamra