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

CMD: Add json output on endpoint config #3234

Merged
merged 1 commit into from
Mar 21, 2018
Merged

Conversation

eloycoto
Copy link
Member

I saw, a few times, issues where the config cannot be retrieved
correctly because endpoint config does not have -o json support.

Added json support to stop failing in the test. Related with failures in
Conntrack-related configuration options for endpoints test.

Signed-off-by: Eloy Coto eloy.coto@gmail.com

I saw, a few times, issues  where the config cannot be retrieved
correctly because `endpoint config` does not have `-o json` support.

Added json support to stop failing in the test. Related with failures in
`Conntrack-related configuration options for endpoints` test.

Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
@eloycoto eloycoto added pending-review kind/bug/CI This is a bug in the testing code. area/CI Continuous Integration testing issue or flake labels Mar 21, 2018
@eloycoto eloycoto requested review from a team as code owners March 21, 2018 17:58
@eloycoto
Copy link
Member Author

test-me-please

@ianvernon
Copy link
Member

@eloycoto what were the failures you saw, exactly? Has this been linked to build failures in master?

@eloycoto
Copy link
Member Author

@ianvernon

From:

https://jenkins.cilium.io/job/cilium-ginkgo/job/cilium/job/master/415/

time="2018-03-21T11:03:07Z" level=debug msg="running command: sudo cilium endpoint config 13233 -o json | jq '.mutable.ConntrackLocal'"
cmd: "sudo cilium endpoint config 13233 -o json | jq '.mutable.ConntrackLocal'" exitCode: 4 
 Error: unknown shorthand flag: 'o' in -o
Usage:
  cilium endpoint config <endpoint id> [<option>=(enable|disable) ...] [flags]

Examples:
endpoint config 5421 DropNotification=false TraceNotification=false

Flags:
      --list-options   List available options

Global Flags:
      --config string   config file (default is $HOME/.cilium.yaml)
  -D, --debug           Enable debug messages
  -H, --host string     URI to server-side API

parse error: Invalid numeric literal at line 1, column 8

@ianvernon
Copy link
Member

@eloycoto thanks! This is quite worrisome to me :(
Rhetorical question: how was the test ever working if -o json wasn't an option for the command?

@eloycoto
Copy link
Member Author

We always set the config, doesn't matter if the value is the same.

func (s *SSHMeta) EndpointSetConfig(id, option, value string) bool {
	logger := s.logger.WithFields(logrus.Fields{"endpointID": id})
	res := s.ExecCilium(fmt.Sprintf(
		"endpoint config %s -o json | jq '.mutable.%s'", id, option))

	if res.SingleOut() == value {
		logger.Debugf("no need to update %s=%s; value already set", option, value)
		return res.WasSuccessful()
	}

	before := s.EndpointGet(id)
	if before == nil {
		return false
	}

	configCmd := fmt.Sprintf("endpoint config %s %s=%s", id, option, value)
	data := s.ExecCilium(configCmd)
	if !data.WasSuccessful() {
		logger.Errorf("cannot set endpoint configuration %s=%s", option, value)
		return false
	}

	return s.WaitEndpointRegenerated(id)
}

@ianvernon
Copy link
Member

@eloycoto got it, makes more sense.

To avoid this sort of issue in future helper functions, I think as the helpers get more complex, we can use ExpectSuccess() and other such gomega checkers like ExpectWithOffset. This will ensure that for every command we run, even in helper functions, we get the result we expect. We got lucky here.

@eloycoto
Copy link
Member Author

Yes, but I do not really like having Expect in helpers, it means that helpers are no longer helpers, and are part of the test. Helpers should return error if something happens.

I'm thinking in situations that you want to create a container without valid data, or test invalid config options for an endpoint.

If error, it should be logged and returned. IMHO.

@tgraf tgraf merged commit dfeb3fc into cilium:master Mar 21, 2018
@tgraf tgraf added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake kind/bug/CI This is a bug in the testing code. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants