Add the traffic flags to the apply command #1187
Conversation
There was a problem hiding this comment.
@jwilner: 0 warnings.
In response to this:
Description
Adds the traffic flags to the
applycommand.Changes
- Adds the traffic flags to the
applycommand.Reference
Fixes #1186
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
knative-prow-robot
added
the
needs-ok-to-test
|
Hi @jwilner. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
size/XS
jwilner
force-pushed
the
add-traffic-flags-to-apply
branch
from
c78b560
to
80bcdf8
Compare
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
|
@jwilner : The build tests complaining about the missing auto-docs (for new traffic flags), which can be generated by running |
|
Thanks for the tip @navidshaikh -- it should be fixed now. |
|
Sorry for my late reply, I still have to wrap around how and even whether the traffic split should be included into |
|
Hi @rhuss. Thanks for responding. It's certainly not beautiful to have to do a read and all of the parsing to construct the traffic split expression with old versions, but it's possible and useful. You can already control traffic splits with this command via the file; really, this is just providing parity via flags imo. For context, my use case is really very standard I think. I am trying to set up a simple CD pipeline. Like most CD pipelines, we want to track as much as possible the configuration of our service in source control. Using That said, we also want to do some very basic traffic splitting -- we want the ability to "dark deploy" a feature branch of our service, and it would be very cumbersome to have to edit and check in the file definition to achieve that. Permitting the traffic flags with the I think the above is a very standard use case, and I think this change set is necessary to support it (which I can empirically confirm, b/c we're using my fork for CD). I'm pasting below the wrapper I wrote because it may help provide additional context. The material part is the #!/usr/bin/env bash
# A drone plugin which runs in two different logical modes:
# - `deploy` creates a new knative revision serving 100% of traffic; old revisions serving traffic are forgotten.
# - `dark deploy` deploys a new knative service serving 0% of traffic that's exposed via knative tagged header routing;
# they're intended for testing. Triggered by the presence of `route_tag`.
#
# See the README for a full documentation of available parameters.
main() {
[[ -n "${PLUGIN_CREDENTIALS}" ]] || die "credentials is required"
[[ -n "${PLUGIN_PROJECT}" ]] || die "project is required"
[[ -n "${PLUGIN_REGION}" ]] || die "region is required"
[[ -n "${PLUGIN_CLUSTER}" ]] || die "cluster is required"
[[ -z "${PLUGIN_SERVICE_YAML}" ]] && export PLUGIN_SERVICE_YAML=service.yaml
[[ -f "${PLUGIN_SERVICE_YAML}" ]] || die "service_yaml does not exist"
local service_name
service_name="$(yq --exit-status e .metadata.name "${PLUGIN_SERVICE_YAML}")"
export SERVICE_NAME="${service_name}"
if [[ -z "${PLUGIN_DOCKER_REPO}" ]]; then
local repo
repo="$(cut -d/ -f2 <<<"${DRONE_REPO}")"
export PLUGIN_DOCKER_REPO="us.gcr.io/nyt-auth-dev/${repo}"
fi
if [[ -z "${PLUGIN_IMAGE_TAG}" ]] && [[ -f .version ]]; then
PLUGIN_IMAGE_TAG="$(cat .version)"
export PLUGIN_IMAGE_TAG
fi
log_in
deploy
}
die() {
echo "${@}" >&2
exit 1
}
log_in() {
local credentials="${PLUGIN_CREDENTIALS}"
local project="${PLUGIN_PROJECT}"
local region="${PLUGIN_REGION}"
local cluster="${PLUGIN_CLUSTER}"
local creds_file
creds_file="$(mktemp)"
# shellcheck disable=SC2064
trap "rm ${creds_file}" EXIT
echo "${credentials}" >"${creds_file}"
gcloud auth activate-service-account --key-file="${creds_file}"
gcloud container clusters get-credentials --region "${region}" --project "${project}" "${cluster}"
}
deploy() {
local service_yaml="${PLUGIN_SERVICE_YAML}"
local service_name="${SERVICE_NAME}"
local docker_repo="${PLUGIN_DOCKER_REPO}"
local image_tag="${PLUGIN_IMAGE_TAG}"
local route_tag="${PLUGIN_ROUTE_TAG}" # empty if regular release
# Traffic Settings
#
# Knative traffic settings look like:
#
# [{"tag": "foo", "revisionName": "rev2", "percent": 0}, {"revisionName": "rev1", "percent": 100}]
#
# The fields
# - Tag exposes the revision at an alternate URL
# - Revision name is the actual revision
# - Percent is the amount of traffic it receives
#
# Note that a revision does not necessarily have a tag.
#
# We aim to always preserve "dark" revisions. If doing a non-dark release, we:
# - Remove all revisions serving > 0% (other non-dark revisions)
# - Add our revision at 100%
#
# If you wanted to add a new dark mode revision (fizzbuzz) to the service, you need to pass the whole state of the
# world via key-value pairs on the CLI like so:
# --tag @latest=fizzbuzz,rev2=foo --traffic @latest=0,rev2=0,rev1=100
#
# If you wanted to redeploy -- overwrite -- the tag foo, you must first untag the existing revision:
# --untag foo
# And then reapply it (also omitting the old rev from traffic at this point)
# --tag @latest=foo --traffic @latest=0,rev1=100
# load traffic settings, defaulting to empty if service doesn't exist yet
local traffic_settings="[]"
{
local svc_desc
if svc_desc="$(kn service describe --output json "${service_name}" 2>/dev/null)"; then
traffic_settings="$(jq '.status.traffic' <<<"${svc_desc}")"
fi
}
if [[ -z "${route_tag}" ]]; then
traffic_settings="$(
jq '
[{revisionName: "@latest", percent: 100}]
+ [.[] | select(.percent == 0)]
' <<<"${traffic_settings}"
)"
else
# Does the desired tag already exist? If so, delete it:
if jq --exit-status --arg tag "${route_tag}" 'any(.tag == $tag) ' <<<"${traffic_settings}" >/dev/null; then
echo "Untagging old revision with tag ${route_tag}" >&2
kn service update --untag "${route_tag}" "${service_name}"
fi
# Note that we filter out any overwritten tags
traffic_settings="$(
jq --arg tag "${route_tag}" '
[{tag: $tag, revisionName: "@latest", percent: 0}]
+ [.[] | select(.tag != $tag)]
' <<<"${traffic_settings}"
)"
fi
# [{"revisionName": "@latest", "percent": 100}, ...] -> "@latest=100,..."
local traffic
traffic="$(jq --raw-output '[.[] | "\(.revisionName)=\(.percent)"] | join(",")' <<<"${traffic_settings}")"
if [[ "${traffic}" == "@latest=0" ]]; then
echo "Cannot do a deploy if no service has been created yet." >&2
exit 1
fi
local args=(
kn service apply "${service_name}"
--filename "${service_yaml}"
--image "${docker_repo}:${image_tag}"
--revision-name "${service}-${image_tag}-{{.Generation}}-{{.Random 5}}"
--traffic "${traffic}"
)
if [[ -n "${route_tag}" ]]; then
args+=(--label-revision "auth.dev.nyt.net/auto-cleanup=1")
fi
{
local tags
# [{"revisionName": "@latest", "tag": "foo"}, ...] -> "@latest=foo,..."
tags="$(jq --raw-output '[.[] | select(.tag) | "\(.revisionName)=\(.tag)"] | join(",")' <<<"${traffic_settings}")"
if [[ -n "${tags}" ]]; then
args+=(--tag "${tags}")
fi
}
"${args[@]}"
}
main "${@}"
|
|
@jwilner do you have an example of an initial Knative service YAML that defines the traffic split already during creation time (with at least one route that is not |
|
Thanks for the update, and I can see you use case to set the traffic split for an already running service. Still, this it might be trickier than just adding those flags to I still have to play with the flow and the edge cases, but I think for a CD setup it totally makes sense to support the traffic split (which is an odd construct wrt/ to state anyway, so we might add a 'special' behaviour for this part) |
jwilner
force-pushed
the
add-traffic-flags-to-apply
branch
from
09a7cf5
to
3763be0
Compare
knative-prow-robot
added
needs-rebase
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
google-cla
bot
added
cla: no
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
However, this error message looks strange: There is only one target, but an additional tag option when creating the service. I would expect this command to succeed. Omitting the |
|
/retest |
|
tl;dr - I'm positive to integrate the PR as I think it might be useful exactly for the use case that @jwilner mentioned above, and it still can be performed in an idempotent way. The messaging needs to be improved though, as well as the error check for create has some issues. I'm just trying to find out why my tests sometimes hang when running apply and then will jump on the review. |
knative-prow-robot
added
the
needs-rebase
knative-prow-robot
removed
the
needs-rebase
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
knative-prow-robot
removed
the
lifecycle/stale
| waitDoing, waitVerb, err := examineServiceForApply(cmd, client, service.Name) | ||
| if err != nil { | ||
| waitDoing, waitVerb := "Applying", "applied" | ||
| if isCreate, err := examineServiceForApply(cmd, client, service.Name); err != nil { |
There was a problem hiding this comment.
Why not is,ply this if/else/if with simply:
err := examineServiceForApply(cmd, client, service.Name)
if err != nil {
return err
}
// do as in the else / if since the ifCreate condition applies to both parts of the statement
// and just add a `if !isCreate ...` guard and return or do what's needed in that case in a helper function
|
The following is the coverage report on the affected files.
|
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
knative-prow-robot
removed
the
lifecycle/stale
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jwilner The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
the
lifecycle/stale
|
@jwilner: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
github-actions
bot
removed
the
lifecycle/stale
|
@jwilner: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
needs-rebase
|
This Pull Request is stale because it has been open for 90 days with |
github-actions
bot
added
the
lifecycle/stale
Description
Adds the traffic flags to the
applycommand.Changes
applycommand.Reference
Fixes #1186