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 FieldSelectors and LabelSelector for GetPogLogs #800

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

MengmengZHANG
Copy link
Contributor

@MengmengZHANG MengmengZHANG commented Jul 6, 2019

What this PR does / why we need it:
Add more control parameters to PodLogLimits

type PodLogLimits struct {
    Namespaces string 
    SonobuoyNamespace *bool 
    FieldSelectors []string 
    LabelSelector string
    ...
}

When collecting pod logs, the scope of pods is defined by:

(Namespaces OR SonobuoyNamespace OR FieldSelectors) AND LabelSelector

Then for each pod, the format and size of logs is defined by other fields, e.g. SinceSeconds

Which issue(s) this PR fixes

Special notes for your reviewer:
SonobuoyNamespace's default value is true, which causes some problem when merging the user's input config and the default config. A workaround is implemented to avoid this issue:

  • use *bool to differentiate input = nil and input= false
  • only nil input will be overwritten by default config
  • Before the merge, save user's input, and restore a non-nil user's input if the merge library has overwritten it.

Hopefully the merge library provides a better solution for this open issue

To test:

  • sonobuoy gen config > test.config
  • remove plugins from test.config: "Plugins": []
  • adapt the provided parameters, e.g. FieldSelectors
  • sonobuoy run --sonobuoy-image=xxx --config test.config --wait --image-pull-policy=Always

@codecov-io
Copy link

codecov-io commented Jul 6, 2019

Codecov Report

Merging #800 into master will increase coverage by 0.2%.
The diff coverage is 26.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #800     +/-   ##
=========================================
+ Coverage   42.71%   42.91%   +0.2%     
=========================================
  Files          71       71             
  Lines        4198     4229     +31     
=========================================
+ Hits         1793     1815     +22     
- Misses       2298     2308     +10     
+ Partials      107      106      -1
Impacted Files Coverage Δ
pkg/discovery/pods.go 0% <0%> (ø) ⬆️
pkg/discovery/queries.go 13.95% <0%> (-0.17%) ⬇️
cmd/sonobuoy/app/gen.go 54.07% <100%> (+2.13%) ⬆️
pkg/config/config.go 60.65% <100%> (+2.03%) ⬆️
pkg/discovery/discovery.go 5.08% <26.08%> (+5.08%) ⬆️
cmd/sonobuoy/app/status.go 59.34% <0%> (+2.19%) ⬆️
pkg/plugin/aggregation/aggregator.go 82.92% <0%> (+4.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5009540...ef10c32. Read the comment docs.

Copy link
Contributor

@johnSchnake johnSchnake left a comment

Choose a reason for hiding this comment

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

Pretty good; I left lots of comments.

  • needs to reconfigure the flow related to getting the field selectors (no mutating config, easier to understand/test method signature)
  • needs unit testing of new functionality
  • I think some of the docs can be improved slightly

Copy link
Contributor

@johnSchnake johnSchnake left a comment

Choose a reason for hiding this comment

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

Very minor changes requested; looks a lot better IMO now.

I'd have @zubron or @stevesloka do a more thorough look though since I had just a few min. to go over this and haven't tested it manually either.

cmd/sonobuoy/app/gen_test.go Outdated Show resolved Hide resolved
cmd/sonobuoy/app/gen_test.go Outdated Show resolved Hide resolved
pkg/discovery/discovery.go Outdated Show resolved Hide resolved
pkg/discovery/discovery_test.go Outdated Show resolved Hide resolved
pkg/discovery/queries.go Outdated Show resolved Hide resolved
pkg/discovery/queries.go Outdated Show resolved Hide resolved
pkg/discovery/queries.go Outdated Show resolved Hide resolved
pkg/discovery/queries.go Outdated Show resolved Hide resolved
Copy link
Contributor

@johnSchnake johnSchnake left a comment

Choose a reason for hiding this comment

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

LGTM from me but I am unable to manually test this; need follow up review.

I'd like to just make sure someone actually tests the cases of setting one/all/neither of those new fields (field selectors/namespaces/sonobuoyNamespace).

👍 Great job though and thanks for the addition.

@MengmengZHANG
Copy link
Contributor Author

I have squashed the commits into one, and retest all the possible test cases, working fine. I have also provided the commands that I used to test in the pull request summary.

Copy link
Contributor

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

I ran a scan with the following config:

{
    "Description": "DEFAULT",
    "UUID": "178860c5-2d09-4516-9bdc-a3bfc2b825b0",
    "Version": "v0.15.0-18-g0b423b3",
    "ResultsDir": "/tmp/sonobuoy",
    "Resources": ["apiservices", "certificatesigningrequests", "clusterrolebindings", "clusterroles", "componentstatuses", "configmaps", "controllerrevisions", "cronjobs", "customresourcedefinitions", "daemonsets", "deployments", "endpoints", "ingresses", "jobs", "leases", "limitranges", "mutatingwebhookconfigurations", "namespaces", "networkpolicies", "nodes", "persistentvolumeclaims", "persistentvolumes", "poddisruptionbudgets", "pods", "podlogs", "podsecuritypolicies", "podtemplates", "priorityclasses", "replicasets", "replicationcontrollers", "resourcequotas", "rolebindings", "roles", "servergroups", "serverversion", "serviceaccounts", "services", "statefulsets", "storageclasses", "validatingwebhookconfigurations", "volumeattachments"],
    "Filters": {
        "Namespaces": ".*",
        "LabelSelector": ""
    },
    "Limits": {
        "PodLogs": {
            "Namespaces": "",
            "SonobuoyNamespace": true,
            "FieldSelectors": ["metadata.namespace==default"],
            "LabelSelector": "",
            "Previous": false,
            "SinceSeconds": null,
            "SinceTime": null,
            "Timestamps": false,
            "TailLines": null,
            "LimitBytes": null,
            "LimitSize": "",
            "LimitTime": ""
        }
    },
    "QPS": 30,
    "Burst": 50,
    "Server": {
        "bindaddress": "0.0.0.0",
        "bindport": 8080,
        "advertiseaddress": "",
        "timeoutseconds": 10800
    },
    "Plugins": [],
    "PluginSearchPath": ["./plugins.d", "/etc/sonobuoy/plugins.d", "~/sonobuoy/plugins.d"],
    "Namespace": "heptio-sonobuoy",
    "WorkerImage": "gcr.io/heptio-images/sonobuoy:v0.15.0-18-g0b423b3",
    "ImagePullPolicy": "IfNotPresent",
    "ImagePullSecrets": ""
}

I expected to only see podlogs in the default namespace, but I still see all namespaces:
image

site/docs/master/sonobuoy-config.md Outdated Show resolved Hide resolved
@MengmengZHANG
Copy link
Contributor Author

MengmengZHANG commented Jul 10, 2019

There is one single "=" instead of "==". The site/docs/master/sonobuoy-config.md shows examples of the format:
"FieldSelectors": ["metadata.namespace=default"]

Also, after running the scan, check any error in logs:
sonobuoy logs

@stevesloka
Copy link
Contributor

@MengmengZHANG
Copy link
Contributor Author

sorry it's a typo

Are these docs wrong then? I copy/pasted: https://github.com/heptio/sonobuoy/pull/800/files#diff-8f0acbcbc04c588fc62d68fff9d8a5fcR69

@stevesloka
Copy link
Contributor

@MengmengZHANG I took out the double equals and have the same result, still got logs for all namespaces.

@MengmengZHANG
Copy link
Contributor Author

@MengmengZHANG I took out the double equals and have the same result, still got logs for all namespaces.

Seems @stevesloka doesn't use the new container image, will check offline.

@stevesloka
Copy link
Contributor

All seems to work @MengmengZHANG, just need to clean up those docs.

@MengmengZHANG
Copy link
Contributor Author

Thanks @stevesloka , I have updated the docs. Could you merge the pull request?

Copy link
Contributor

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

/lgtm

@stevesloka stevesloka dismissed johnSchnake’s stale review July 10, 2019 17:15

Changes resolved in review.

@stevesloka stevesloka merged commit 700ac79 into vmware-tanzu:master Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query selectors
4 participants