Add istio.io repo #9983

clyang82 · 2018-11-15T08:54:47Z

Signed-off-by: Chun Lin Yang clyang@cn.ibm.com

Add istio.io repo before run helm dep update
Update the istio.io repo to release url in release package
the cmd exits if there is no required args
show usage only when run create_release_archives.sh
Do not create temp fold in the beginning

istio-testing · 2018-11-15T08:55:03Z

Hi @clyang82. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

clyang82 · 2018-11-15T08:55:43Z

/ @linsun @hklai

linsun · 2018-11-15T15:25:53Z

release/create_release_archives.sh

+function replace_with_release_charts_url() {
+  local origin_url="istio-prerelease/daily-build/master-latest-daily/charts"
+  local target_url="istio-release/releases/${VER_STRING}/charts"
+  sed -i.bak "s:${origin_url}:${target_url}:g" "${COMMON_FILES_DIR}/install/kubernetes/helm/istio/README.md"


LGTM, would let @hklai to confirm this is correct urls.

linsun · 2018-11-15T20:07:22Z

/ok-to-test

hklai · 2018-11-16T00:48:54Z

install/kubernetes/helm/istio/README.md

@@ -116,6 +116,11 @@ The chart deploys pods that consume minimum resources as specified in the resour
    EOF
    ```

+1. Add `istio.io` chart repository and point to the release:
+    ```
+    $ helm repo add istio.io https://storage.googleapis.com/istio-prerelease/daily-build/master-latest-daily/charts


Since this is release-1.1, you probably want to use https://storage.googleapis.com/istio-prerelease/daily-build/release-1.1-latest-daily/charts instead.

the code in release-1.1 will be merged back to master. so if change it to release-1.1, it is confused in master. so how about just keep as it is? the released packages have the correct url.

@hklai Does it make sense? Thanks.

The same is true for many other things, like api and proxy shas, and the versions hardcoded in some helm config.

I suppose we should keep this specific to release-1.1 as it might have (or will be) diverged from master.

okay. let us use https://storage.googleapis.com/istio-prerelease/daily-build/release-1.1-latest-daily/charts instead.

hklai · 2018-11-16T00:49:10Z

release/create_release_archives.sh

@@ -15,16 +15,12 @@
 #
 ################################################################################

-set -o errexit


why do you move these down?

set -o errexit set -o pipefail set -x

Move them together to ensure perform this cmd create_release_archives.sh only show the usage without unnecessary information

Before changes:

chunlins-mbp:istio clyang$ ./release/create_release_archives.sh ++ mktemp -d /tmp/istio.version.XXXX + TEMP_DIR=/tmp/istio.version.wORR + BASE_DIR=/tmp/istio.version.wORR + ISTIOCTL_SUBDIR=istioctl + OUTPUT_PATH= + VER_STRING= + getopts d:i:o:v: arg + [[ -z /tmp/istio.version.wORR ]] + [[ -z '' ]] + usage + echo './release/create_release_archives.sh -d <path> path to use for temp directory (optional, randomized default is /tmp/istio.version.wORR ) -o <path> path where build output/artifacts are stored (required) -i <name> subdirectory in -o path to use for istioctl (optional) -v <ver> version info to include in filename (e.g., 1.0) (required)' ./release/create_release_archives.sh -d <path> path to use for temp directory (optional, randomized default is /tmp/istio.version.wORR ) -o <path> path where build output/artifacts are stored (required) -i <name> subdirectory in -o path to use for istioctl (optional) -v <ver> version info to include in filename (e.g., 1.0) (required) + exit 1

After Changes:

chunlins-mbp:istio clyang$ ./release/create_release_archives.sh ./release/create_release_archives.sh -d <path> path to use for temp directory (optional, randomized default is /tmp/istio.version.XXXX ) -o <path> path where build output/artifacts are stored (required) -i <name> subdirectory in -o path to use for istioctl (optional) -v <ver> version info to include in filename (e.g., 1.0) (required)

Let's ignore those for now. This file is not mean to be used by human directly anyway, and @rkpagadala is moving stuff around.

I suggest we just revert.

@hklai If we can make it use by human directly, why not. At least, when I did test the release package, I need to run this Shell to generate the package. Then show the usage more friendly is not so bad. What do you think?

OK it's fine to make it more usable.

hklai · 2018-11-16T00:49:21Z

release/create_release_archives.sh

 # This script primarily exists for Cloud Builder.  This script
 # reads artifacts from a specified directory, generates tar files
 # based on those artifacts, and then stores the tar files
 # back to the directory.

-TEMP_DIR="$(mktemp -d /tmp/istio.version.XXXX)"


mktemp will be performed always in the beginning. but rm -rf "$TEMP_DIR" in the end. sometimes, there is error in the middle so that the "$TEMP_DIR" cannot be removed as expected.

I think we want to just a clean and new temp folder always to avoid confusion. Cleaning up temp files is typically best effort.

see this example:

chunlins-mbp:istio clyang$ ./release/create_release_archives.sh ++ mktemp -d /tmp/istio.version.XXXX + TEMP_DIR=/tmp/istio.version.wORR + BASE_DIR=/tmp/istio.version.wORR + ISTIOCTL_SUBDIR=istioctl + OUTPUT_PATH= + VER_STRING= + getopts d:i:o:v: arg + [[ -z /tmp/istio.version.wORR ]] + [[ -z '' ]] + usage + echo './release/create_release_archives.sh -d <path> path to use for temp directory (optional, randomized default is /tmp/istio.version.wORR ) -o <path> path where build output/artifacts are stored (required) -i <name> subdirectory in -o path to use for istioctl (optional) -v <ver> version info to include in filename (e.g., 1.0) (required)' ./release/create_release_archives.sh -d <path> path to use for temp directory (optional, randomized default is /tmp/istio.version.wORR ) -o <path> path where build output/artifacts are stored (required) -i <name> subdirectory in -o path to use for istioctl (optional) -v <ver> version info to include in filename (e.g., 1.0) (required) + exit 1

The temp folder is created but never be cleaned.

You can add a trap EXIT to make sure cleanup happens even in error path

hklai · 2018-11-16T00:51:04Z

release/create_release_archives.sh

@@ -45,6 +41,11 @@ function error_exit() {
  exit "${2:-1}"
 }

+# since there are 2 required options, should show usage and exit with no args specified


I think the following does exactly that?

hklai · 2018-11-16T00:51:29Z

release/create_release_archives.sh

 [[ -z "${BASE_DIR}"    ]] && usage
 [[ -z "${OUTPUT_PATH}" ]] && usage
 [[ -z "${VER_STRING}"  ]] && usage

+if ("${BASE_DIR}" = "${TEMP_DIR}"); then
+  mktemp -d "${TEMP_DIR}"


should have been created above?

hklai · 2018-11-16T00:57:43Z

release/create_release_archives.sh

@@ -69,6 +77,13 @@ mkdir -p "${BIN_DIR}"
 CP=${CP:-"cp"}
 TAR=${TAR:-"tar"}

+function replace_with_release_charts_url() {


it looks like you want do perform this replacement for monthly/LTS/patch release only, but not daily releases, which are stored in the prerelease directory only.

Could you point me out which flag can be used to check it is not a daily release? Thanks.

@clyang82 you can check if
CB_PIPELINE_TYPE == monthly
you will also need to a line at https://github.com/istio/istio/blob/master/prow/release-test.sh#L47 declaring type as daily
export CB_PIPELINE_TYPE=daily

@rkpagadala Thanks for your help. I have updated accordingly.

codecov · 2018-11-21T02:13:39Z

Codecov Report

Merging #9983 into release-1.1 will increase coverage by 3%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           release-1.1   #9983     +/-   ##
=============================================
+ Coverage           68%     70%     +3%     
=============================================
  Files              571     442    -129     
  Lines            48706   41352   -7354     
=============================================
- Hits             32982   28878   -4104     
+ Misses           13946   11064   -2882     
+ Partials          1778    1410    -368

Impacted Files	Coverage Δ
mixer/adapter/inventory.gen.go	`0% <0%> (-100%)`	⬇️
pilot/pkg/networking/plugin/mixer/mixer.go	`1% <0%> (-77%)`	⬇️
mixer/adapter/rbac/rbac.go	`0% <0%> (-70%)`	⬇️
galley/pkg/meshconfig/cache.go	`0% <0%> (-63%)`	⬇️
pkg/mcp/server/monitoring.go	`3% <0%> (-57%)`	⬇️
mixer/pkg/runtime/config/queries.go	`64% <0%> (-36%)`	⬇️
mixer/pkg/runtime/config/snapshot.go	`58% <0%> (-36%)`	⬇️
pilot/pkg/serviceregistry/memory/discovery.go	`57% <0%> (-29%)`	⬇️
pilot/pkg/proxy/envoy/discovery.go	`0% <0%> (-29%)`	⬇️
mixer/adapter/stackdriver/stackdriver.go	`50% <0%> (-28%)`	⬇️
... and 271 more

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 c24863b...ee90930. Read the comment docs.

hklai · 2018-11-27T18:56:30Z

prow/release-test.sh

@@ -45,6 +45,7 @@ cat << EOF > "/workspace/gcb_env.sh"
 export CB_BRANCH="${GIT_SHA}"
 export CB_VERSION="${GIT_SHA}"
 export CB_ISTIOCTL_DOCKER_HUB="docker.io/istio"
+export CB_PIPELINE_TYPE=daily


maybe we need two tests for the two types?

for test coverage purpose, I think we should have two tests for the two types. but considering the 2 factors:

there is no many differences for the two types right now.

add one more test needs spend extra 13 minutes.
Maybe we can consider in the future.
@hklai @rkpagadala

clyang82 · 2018-11-28T14:48:27Z

@hklai This PR should be included into release-1.1. Could you please approve and help to merge if you have no more comments? Thanks.

costinm · 2018-11-29T01:02:44Z

install/kubernetes/helm/istio/README.md

@@ -116,6 +116,11 @@ The chart deploys pods that consume minimum resources as specified in the resour
    EOF
    ```

+1. Add `istio.io` chart repository and point to the release:
+    ```
+    $ helm repo add istio.io https://storage.googleapis.com/istio-prerelease/daily-build/release-1.1-latest-daily/charts


What will be the URL for production users ? We can't use prerelease and storage.

it will be replaced during creating release package.

if [ "${CB_PIPELINE_TYPE}" = "monthly" ]; then local origin_url="istio-prerelease/daily-build/master-latest-daily/charts" local target_url="istio-release/releases/${VER_STRING}/charts" sed -i.bak "s:${origin_url}:${target_url}:g" "${COMMON_FILES_DIR}/install/kubernetes/helm/istio/README.md" rm -rf "${COMMON_FILES_DIR}/install/kubernetes/helm/istio/README.md.bak" fi }

@clyang82 Can you please add some comments to production users here? +1 to @costinm , it seems to be weird to use pre release here.

costinm · 2018-11-29T01:05:01Z

Can you explain why the change is made - I'm not sure I follow.

It looks like we are moving towards operator and away from depending on specific helm features - I think we need to re-evaluate if we even want the 'helm dep update' in 1.1, or move back the subcharts like in 1.0

If I remember correctly, it was added for cni - but for CNI we can simply treat it as a separate module ( consistent with the 'modularization' model). It seems we agreed on multi-step install , and for 'beginner' case we'll not need CNI.

costinm · 2018-11-29T01:05:09Z

/hold

clyang82 · 2018-11-29T01:14:02Z

@costinm right now, based on release-1.1, we have to run helm dep update for installation. it will throws error something like cannot find istio.io so that we need to run helm repo add istio.io xxx. we have different location for release and prerelease. that is why have the change.

hklai · 2018-11-29T19:49:37Z

release/create_release_archives.sh

@@ -154,9 +175,9 @@ ls -l  "${COMMON_FILES_DIR}/install/kubernetes/helm/istio"
 # Changing dir such that tar and zip files are
 # created with right hiereachy
 pushd "${COMMON_FILES_DIR}/.."
+replace_with_release_charts_url


Can this be done in https://github.com/istio/istio/blob/master/release/gcb/modify_values.sh instead?

That is where we modify all helm related values.

sure. done. Thanks.

istio-testing · 2018-11-30T05:21:23Z

@clyang82: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow//e2e-simpleTests-minProfile.sh	16a04a66652637584d6df05cd9c6c6e185ac5ba3	link	`/test e2e-simpleTestsMinProfile`
prow/e2e-mixer-no_auth-mcp.sh	16a04a66652637584d6df05cd9c6c6e185ac5ba3	link	`/test e2e-mixer-no_auth-mcp`

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.

clyang82 · 2018-11-30T06:16:49Z

/test test
/test e2e-mixer-noauth-v1alpha3-v2

hklai

/lgtm

clyang82 · 2018-12-01T07:08:12Z

@costinm can you please remove the hold flag if you are OK with my changes? Thanks

clyang82 · 2018-12-01T07:08:23Z

/test test
/test e2e-mixer-noauth-v1alpha3-v2

stale · 2018-12-15T08:26:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

clyang82 · 2018-12-16T14:17:10Z

@costinm I think this PR must be in release 1.1. any comments? Could you please remove the hold flag?

clyang82 · 2018-12-20T00:42:58Z

@linsun @hklai Can you help to merge this PR? Thanks.

clyang82 · 2018-12-28T05:14:36Z

/retest

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

linsun · 2019-01-02T16:18:07Z

/lgtm

/approve

istio-testing · 2019-01-02T16:18:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clyang82, hklai, linsun

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:

~~install/OWNERS~~ [hklai,linsun]
~~prow/OWNERS~~ [hklai,linsun]
~~release/OWNERS~~ [hklai,linsun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes label Nov 15, 2018

istio-testing requested review from gyliu513 and rkpagadala November 15, 2018 08:54

istio-testing added the needs-ok-to-test label Nov 15, 2018

clyang82 mentioned this pull request Nov 15, 2018

Add istio.io repo #9920

Merged

linsun reviewed Nov 15, 2018

View reviewed changes

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 15, 2018

hklai reviewed Nov 16, 2018

View reviewed changes

clyang82 mentioned this pull request Nov 20, 2018

REQUEST: New membership for clyang82 istio/community#102

Closed

7 tasks

hklai reviewed Nov 27, 2018

View reviewed changes

costinm reviewed Nov 29, 2018

View reviewed changes

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Nov 29, 2018

hklai reviewed Nov 29, 2018

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 30, 2018

clyang82 force-pushed the add_istio.io_repo branch from 9fb307a to 9d8a4c6 Compare November 30, 2018 04:45

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 30, 2018

hklai approved these changes Nov 30, 2018

View reviewed changes

stale bot added the stale label Dec 15, 2018

stale bot removed the stale label Dec 16, 2018

hklai removed the do-not-merge/hold Block automatic merging of a PR. label Dec 20, 2018

clyang82 added 5 commits December 28, 2018 17:05

Add isito.io repo

9acc85f

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

Add monthly flag to replace

84ff5bf

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

address review comments

7317c11

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

move modify helm values into modify_values.sh

1fa6bef

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

add back missed changes

ee90930

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

clyang82 force-pushed the add_istio.io_repo branch from 16a04a6 to ee90930 Compare December 28, 2018 09:06

istio-testing added the approved label Dec 28, 2018

istio-testing assigned linsun Jan 2, 2019

istio-testing added the lgtm label Jan 2, 2019

linsun merged commit 45333c9 into istio:release-1.1 Jan 2, 2019

clyang82 deleted the add_istio.io_repo branch January 11, 2019 07:12

Add istio.io repo #9983

Add istio.io repo #9983

Conversation

clyang82 commented Nov 15, 2018

istio-testing commented Nov 15, 2018

clyang82 commented Nov 15, 2018

Choose a reason for hiding this comment

linsun commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 21, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

clyang82 Nov 28, 2018 • edited

Choose a reason for hiding this comment

clyang82 commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costinm commented Nov 29, 2018

costinm commented Nov 29, 2018

clyang82 commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-testing commented Nov 30, 2018

clyang82 commented Nov 30, 2018

hklai left a comment

Choose a reason for hiding this comment

clyang82 commented Dec 1, 2018

clyang82 commented Dec 1, 2018 • edited

stale bot commented Dec 15, 2018

clyang82 commented Dec 16, 2018

clyang82 commented Dec 20, 2018

clyang82 commented Dec 28, 2018

linsun commented Jan 2, 2019

istio-testing commented Jan 2, 2019

codecov bot commented Nov 21, 2018 •

edited

clyang82 Nov 28, 2018 •

edited

clyang82 commented Dec 1, 2018 •

edited