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

Ensure that "antctl version" always outputs the client version #1876

Conversation

antoninbas
Copy link
Contributor

Update antctl command framework to support outputting information to the
user even when a client request to the server (Agent or Controller)
fails. We leverage this framework to ensure that "antctl version" always
outputs the client version.

To test this functionality, we introduce an interface for the "client"
part of antctl, which is in charge of sending a request to the
appropriate server. We mock this interface so it can be used in unit
tests. Although in the Antrea codebase, we typically place mocks in a
sub-package called "testing", it is not possible here, due to the very
flat nature of the antctl package, and to the dependencies between
objects. For example, "commandDefinition" depends on "client" and
"client" depends on "commandDefinition". Changing this structure would
require a large code refactor, so instead we adapt the code generation
framework to exceptionally tolerate placing mocks directly in the
package.

Fixes #1872

Update antctl command framework to support outputting information to the
user even when a client request to the server (Agent or Controller)
fails. We leverage this framework to ensure that "antctl version" always
outputs the client version.

To test this functionality, we introduce an interface for the "client"
part of antctl, which is in charge of sending a request to the
appropriate server. We mock this interface so it can be used in unit
tests. Although in the Antrea codebase, we typically place mocks in a
sub-package called "testing", it is not possible here, due to the very
flat nature of the antctl package, and to the dependencies between
objects. For example, "commandDefinition" depends on "client" and
"client" depends on "commandDefinition". Changing this structure would
require a large code refactor, so instead we adapt the code generation
framework to exceptionally tolerate placing mocks directly in the
package.

Fixes antrea-io#1872
@antoninbas antoninbas force-pushed the print-antctl-client-version-even-if-server-request-fails branch from c57b6fc to a77a8ed Compare February 17, 2021 02:36
@codecov-io
Copy link

codecov-io commented Feb 17, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@f93242a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1876   +/-   ##
=======================================
  Coverage        ?   43.19%           
=======================================
  Files           ?      200           
  Lines           ?    17244           
  Branches        ?        0           
=======================================
  Hits            ?     7449           
  Misses          ?     8777           
  Partials        ?     1018           
Flag Coverage Δ
e2e-tests 43.19% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

Could you paste the example output for me to understand?

@antoninbas
Copy link
Contributor Author

New anctl behavior:

$ ./bin/antctl-darwin -o json version
{
  "antctlVersion": "v0.14.0-dev-d3473075.dirty"
}
Error: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
$ ./bin/antctl-darwin --kubeconfig ~/vmware/antrea/test/e2e/infra/vagrant/playbook/kube/config -o json version
{
  "controllerVersion": "v0.13.0-dev-85ceb847",
  "antctlVersion": "v0.14.0-dev-d3473075.dirty"
}

And kubectl for comparison:

$ kubectl -o json version
{
  "clientVersion": {
    "major": "1",
    "minor": "20",
    "gitVersion": "v1.20.1",
    "gitCommit": "c4d752765b3bbac2237bf87cf0b1c2e307844666",
    "gitTreeState": "clean",
    "buildDate": "2020-12-18T12:09:25Z",
    "goVersion": "go1.15.5",
    "compiler": "gc",
    "platform": "darwin/amd64"
  }
}
The connection to the server localhost:8080 was refused - did you specify the right host or port?
$ kubectl --kubeconfig ~/vmware/antrea/test/e2e/infra/vagrant/playbook/kube/config -o json version
{
  "clientVersion": {
    "major": "1",
    "minor": "20",
    "gitVersion": "v1.20.1",
    "gitCommit": "c4d752765b3bbac2237bf87cf0b1c2e307844666",
    "gitTreeState": "clean",
    "buildDate": "2020-12-18T12:09:25Z",
    "goVersion": "go1.15.5",
    "compiler": "gc",
    "platform": "darwin/amd64"
  },
  "serverVersion": {
    "major": "1",
    "minor": "20",
    "gitVersion": "v1.20.2",
    "gitCommit": "faecb196815e248d3ecfb03c680a4507229c2a56",
    "gitTreeState": "clean",
    "buildDate": "2021-01-13T13:20:00Z",
    "goVersion": "go1.15.5",
    "compiler": "gc",
    "platform": "linux/amd64"
  }
}

Both exit with code 1 in the error / incomplete case.

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

Thanks for pasting the outputs. LGTM.

)

func RequestErrorFallback() (io.Reader, error) {
return strings.NewReader("{}"), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the Transform func will still be called for the fallback response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, I considered several approaches but that's the one I went for: generate a "fake" server response and let it go through the transform functions as usual. An alternative was to modify the transforms to handle nil responses, but that seemed more error-prone, more invasive and less flexible.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas merged commit 1f7775e into antrea-io:main Feb 24, 2021
@antoninbas antoninbas deleted the print-antctl-client-version-even-if-server-request-fails branch February 24, 2021 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

antctl version should return cli version without a server connection
5 participants