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.

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

add more neg tests #829

Merged
merged 1 commit into from
Aug 20, 2019
Merged

add more neg tests #829

merged 1 commit into from
Aug 20, 2019

Conversation

freehan
Copy link
Contributor

@freehan freehan commented Aug 17, 2019

migrate part of the test from OSS k8s

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2019
@freehan freehan force-pushed the neg-test branch 4 times, most recently from 65b6f94 to 6bd623a Compare August 17, 2019 00:43
port80 := intstr.FromInt(80)

ctx := context.Background()
numForwardingRules := 1
Copy link
Member

Choose a reason for hiding this comment

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

const

}

scaleAndValidate(2)

Copy link
Member

Choose a reason for hiding this comment

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

don't need newlines here...

Copy link
Member

Choose a reason for hiding this comment

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

why this sequence? is there something specific that is being tested here (document in a comment)


// CheckDistinctResponseHost issue GET call to the vip for 100 times, parse the reponses and calculate the number of distinct backends.
func CheckDistinctResponseHost(vip string, expectDistinctHosts int, tolerateTransientError bool) error {
errs := []error{}
Copy link
Member

Choose a reason for hiding this comment

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

var errs []error

// CheckDistinctResponseHost issue GET call to the vip for 100 times, parse the reponses and calculate the number of distinct backends.
func CheckDistinctResponseHost(vip string, expectDistinctHosts int, tolerateTransientError bool) error {
errs := []error{}
repeat := 100
Copy link
Member

Choose a reason for hiding this comment

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

const

}
wantedReplicas := *deployment.Spec.Replicas

if deployment.Status.Replicas != wantedReplicas {
Copy link
Member

Choose a reason for hiding this comment

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

you could do to avoid mismatches

for _, f := range []struct{
  v *int
  name string
  want int
}{
  {&deployment.Status.Replicas, "replicas", wantedReplicas},
  {&deployment.Status.ReadyReplicas, "ready replicas", wantedReplicas},
  {&deployment.Status.AvailabledReplicas, "available replicas", wantedReplicas},
  {&deployment.Status.UpdatedReplicas, "updated replicas", wantedReplicas},
  ...
}{
  if *f.v != f.want {
    return fmt.Errorf("deployment %s/%s has %d %s, want %d",  deployment.Namespace, deployment.Name, *f.v, f.name, f.want)
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@freehan
Copy link
Contributor Author

freehan commented Aug 19, 2019

Fixed. Ready for another round.

@@ -189,3 +236,16 @@ func DeleteGCPAddress(s *Sandbox, name string) error {

return nil
}

func GetNegStatus(s *Sandbox, name string) (annotations.NegStatus, bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

seems weird to return both bool and error?

}

if deployment.Status.UnavailableReplicas != 0 {
return fmt.Errorf("deployment %s/%s has %v unavailable replicas, want 0", deployment.Namespace, deployment.Name, deployment.Status.UnavailableReplicas)
Copy link
Member

Choose a reason for hiding this comment

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

seems like this would just be?

{&deployment.Status.UnavailableReplicas, "unavailable replicas", 0},


// CheckNegs checks if the network endpoints in the NEGs is in expected state
func CheckNegs(negs map[meta.Key]*fuzz.NetworkEndpoints, expectHealthy bool, expectCount int) error {
count := 0
Copy link
Member

Choose a reason for hiding this comment

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

var (
  count int
  errs []error
  negNames []string
  )

@bowei
Copy link
Member

bowei commented Aug 20, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, freehan

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:

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

@k8s-ci-robot k8s-ci-robot merged commit e3bdc95 into kubernetes:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants