-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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.
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
Introduce the simplest RestartPolicy and handling. #1147
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,9 +58,10 @@ type ContainerManifest struct { | |
// TODO: UUID on Manifest is deprecated in the future once we are done | ||
// with the API refactoring. It is required for now to determine the instance | ||
// of a Pod. | ||
UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` | ||
Volumes []Volume `yaml:"volumes" json:"volumes"` | ||
Containers []Container `yaml:"containers" json:"containers"` | ||
UUID string `yaml:"uuid,omitempty" json:"uuid,omitempty"` | ||
Volumes []Volume `yaml:"volumes" json:"volumes"` | ||
Containers []Container `yaml:"containers" json:"containers"` | ||
RestartPolicy RestartPolicy `json:"restartPolicy,omitempty" yaml:"restartPolicy,omitempty"` | ||
} | ||
|
||
// ContainerManifestList is used to communicate container manifests to kubelet. | ||
|
@@ -254,19 +255,21 @@ const ( | |
// PodInfo contains one entry for every container with available info. | ||
type PodInfo map[string]docker.Container | ||
|
||
// RestartPolicyType represents a restart policy for a pod. | ||
type RestartPolicyType string | ||
type RestartPolicyAlways struct{} | ||
|
||
// Valid restart policies defined for a PodState.RestartPolicy. | ||
const ( | ||
RestartAlways RestartPolicyType = "RestartAlways" | ||
RestartOnFailure RestartPolicyType = "RestartOnFailure" | ||
RestartNever RestartPolicyType = "RestartNever" | ||
) | ||
// TODO(dchen1107): Define what kinds of failures should restart. | ||
// TODO(dchen1107): Decide whether to support policy knobs, and, if so, which ones. | ||
type RestartPolicyOnFailure struct{} | ||
|
||
type RestartPolicyNever struct{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't have any options for anything, then once again I feel like I must vote for strings over the empty policy struct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lavalamp How about we leave this PR as is, and then have the debate about the policy struct. v1beta3 will be a breaking change. We can change it then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I declare the bikeshedding over, for now. |
||
|
||
type RestartPolicy struct { | ||
// Optional: Defaults to "RestartAlways". | ||
Type RestartPolicyType `yaml:"type,omitempty" json:"type,omitempty"` | ||
// Only one of the following restart policies may be specified. | ||
// If none of the following policies is specified, the default one | ||
// is RestartPolicyAlways. | ||
Always *RestartPolicyAlways `json:"always,omitempty" yaml:"always,omitempty"` | ||
OnFailure *RestartPolicyOnFailure `json:"onFailure,omitempty" yaml:"onFailure,omitempty"` | ||
Never *RestartPolicyNever `json:"never,omitempty" yaml:"never,omitempty"` | ||
} | ||
|
||
// PodState is the state of a pod, used as either input (desired state) or output (current state). | ||
|
@@ -283,8 +286,7 @@ type PodState struct { | |
// upon. | ||
// TODO: Make real decisions about what our info should look like. Re-enable fuzz test | ||
// when we have done this. | ||
Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` | ||
RestartPolicy RestartPolicy `json:"restartpolicy,omitempty" yaml:"restartpolicy,omitempty"` | ||
Info PodInfo `json:"info,omitempty" yaml:"info,omitempty"` | ||
} | ||
|
||
// PodList is a list of Pods. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,6 +216,40 @@ func TestValidateContainers(t *testing.T) { | |
} | ||
} | ||
|
||
func TestValidateRestartPolicy(t *testing.T) { | ||
successCases := []api.RestartPolicy{ | ||
{}, | ||
{Always: &api.RestartPolicyAlways{}}, | ||
{OnFailure: &api.RestartPolicyOnFailure{}}, | ||
{Never: &api.RestartPolicyNever{}}, | ||
} | ||
for _, policy := range successCases { | ||
if errs := validateRestartPolicy(&policy); len(errs) != 0 { | ||
t.Errorf("expected success: %v", errs) | ||
} | ||
} | ||
|
||
errorCases := []api.RestartPolicy{ | ||
{Always: &api.RestartPolicyAlways{}, Never: &api.RestartPolicyNever{}}, | ||
{Never: &api.RestartPolicyNever{}, OnFailure: &api.RestartPolicyOnFailure{}}, | ||
} | ||
for k, policy := range errorCases { | ||
if errs := validateRestartPolicy(&policy); len(errs) == 0 { | ||
t.Errorf("expected failure for %s", k) | ||
} | ||
} | ||
|
||
noPolicySpecified := api.RestartPolicy{} | ||
errs := validateRestartPolicy(&noPolicySpecified) | ||
if len(errs) != 0 { | ||
t.Errorf("expected success: %v", errs) | ||
} | ||
if noPolicySpecified.Always == nil { | ||
t.Errorf("expected Always policy specified") | ||
} | ||
|
||
} | ||
|
||
func TestValidateManifest(t *testing.T) { | ||
successCases := []api.ContainerManifest{ | ||
{Version: "v1beta1", ID: "abc"}, | ||
|
@@ -286,8 +320,13 @@ func TestValidatePod(t *testing.T) { | |
"foo": "bar", | ||
}, | ||
DesiredState: api.PodState{ | ||
Manifest: api.ContainerManifest{Version: "v1beta1", ID: "abc"}, | ||
RestartPolicy: api.RestartPolicy{Type: "RestartAlways"}, | ||
Manifest: api.ContainerManifest{ | ||
Version: "v1beta1", | ||
ID: "abc", | ||
RestartPolicy: api.RestartPolicy{ | ||
Always: &api.RestartPolicyAlways{}, | ||
}, | ||
}, | ||
}, | ||
}) | ||
if len(errs) != 0 { | ||
|
@@ -312,8 +351,12 @@ func TestValidatePod(t *testing.T) { | |
"foo": "bar", | ||
}, | ||
DesiredState: api.PodState{ | ||
Manifest: api.ContainerManifest{Version: "v1beta1", ID: "abc"}, | ||
RestartPolicy: api.RestartPolicy{Type: "WhatEver"}, | ||
Manifest: api.ContainerManifest{ | ||
Version: "v1beta1", | ||
ID: "abc", | ||
RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird line wrapping? don't try to pakc it. |
||
Never: &api.RestartPolicyNever{}}, | ||
}, | ||
}, | ||
}) | ||
if len(errs) != 1 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,8 +105,12 @@ func TestExtractFromHTTP(t *testing.T) { | |
manifests: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, | ||
expected: CreatePodUpdate(kubelet.SET, | ||
kubelet.Pod{ | ||
Name: "foo", | ||
Manifest: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, | ||
Name: "foo", | ||
Manifest: api.ContainerManifest{ | ||
Version: "v1beta1", | ||
ID: "foo", | ||
RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed for the test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
}, | ||
}), | ||
}, | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't feel like this switch to using types instead of an enum is good, given the tools we have. The enum was simple and clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're basically counting on the json package to encode the difference between a null and a struct with no contents, which I'm uncomfortable with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to do it this way, I think you have to embed inside a runtime.Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment above to @csrwng. I made this change based on Tim's comment in a separate thread. We had this kind of arguments a lot internally before. :-)
I am looking into runtime.Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel quite strongly that we shouldn't do it this way. Explanation later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this exactly the pattern we use for things like LivenessProbe and Handler (and didn't we just argue Brendan out of a 'type' field?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed IRL - we'll do this for now and propose a more general approach later.