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

helm chart: add log and accountid #69

Merged
merged 1 commit into from
May 21, 2021

Conversation

surajkota
Copy link
Member

@surajkota surajkota commented May 20, 2021

Issue #, if available: aws-controllers-k8s/community#787

Tested using sagemaker-controller

k get pods -n ack-system -ojson

"spec": {
                "containers": [
                    {
                        "args": [
                            "--aws-account-id",
                            "$(AWS_ACCOUNT_ID)",
                            "--aws-region",
                            "$(AWS_REGION)",
                            "--enable-development-logging",
                            "$(ACK_ENABLE_DEVELOPMENT_LOGGING)",
                            "--log-level",
                            "$(ACK_LOG_LEVEL)",
                            "--resource-tags",
                            "$(ACK_RESOURCE_TAGS)",
                            "--watch-namespace",
                            "$(ACK_WATCH_NAMESPACE)"
                        ],
                        "command": [
                            "./bin/controller"
                        ],
                        "env": [
                            {
                                "name": "K8S_NAMESPACE",
                                "valueFrom": {
                                    "fieldRef": {
                                        "apiVersion": "v1",
                                        "fieldPath": "metadata.namespace"
                                    }
                                }
                            },
                            {
                                "name": "AWS_ACCOUNT_ID",
                                "value": "xxx"
                            },
                            {
                                "name": "AWS_REGION",
                                "value": "us-west-2"
                            },
                            {
                                "name": "ACK_WATCH_NAMESPACE"
                            },
                            {
                                "name": "ACK_ENABLE_DEVELOPMENT_LOGGING",
                                "value": "true"
                            },
                            {
                                "name": "ACK_LOG_LEVEL",
                                "value": "INFO"
                            },
                            {
                                "name": "ACK_RESOURCE_TAGS",
                                "value": "services.k8s.aws/managed=true,services.k8s.aws/created=%UTCNOW%,services.k8s.aws/namespace=%KUBERNETES_NAMESPACE%"
                            },
                            {
                                "name": "AWS_ROLE_ARN",
                                "value": "arn:aws:iam::xxx:role/ack-sage-role-ack-bug-bash-may18"
                            },
                            {
                                "name": "AWS_WEB_IDENTITY_TOKEN_FILE",
                                "value": "/var/run/secrets/eks.amazonaws.com/serviceaccount/token"
                            }
                        ],

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@surajkota surajkota requested review from jaypipes and a team May 20, 2021 02:52
@ack-bot ack-bot requested review from a-hilaly and RedbackThomson and removed request for a team May 20, 2021 02:52
# log level for the controller
log:
enable_development_logging: true
log_level: info
Copy link
Member Author

Choose a reason for hiding this comment

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

DEBUG did not work, hence I kept it as info and not INFO

Copy link
Contributor

Choose a reason for hiding this comment

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

The ./kind-build-test.sh script has this set to debug, but I don't think that's appropriate for production use anyway. I think this is good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I am inclined to keep default as debug because info logs are very less and hard to relate with state of resource. I think even the delta is not printed in info. Let's see what others have to say.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I am inclined to keep default as debug because info logs are very less and hard to relate with state of resource. I think even the delta is not printed in info. Let's see what others have to say.

Delta is shown in Info, actually: https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L251

I would prefer to use info level for default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@surajkota is correct. We are logging the diff/delta using V(1) (debug) not info...

That said, I would prefer to merge this with info as the default log level and address specific log messages (and moving some to info vs debug, etc) in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I can create a separate issue/pr for that

@surajkota surajkota self-assigned this May 20, 2021
@surajkota surajkota added the kind/enhancement Categorizes issue or PR as related to existing feature enhancements. label May 20, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented May 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RedbackThomson, surajkota

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

@a-hilaly
Copy link
Member

@RedbackThomson PTAL at my comments on aws-controllers-k8s/test-infra#27

/hold

@ack-bot ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2021
templates/helm/values.yaml.tpl Outdated Show resolved Hide resolved
@a-hilaly
Copy link
Member

/unhold

@ack-bot ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 20, 2021
@RedbackThomson
Copy link
Contributor

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2021
@ack-bot ack-bot merged commit e4f3cda into aws-controllers-k8s:main May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/enhancement Categorizes issue or PR as related to existing feature enhancements. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants