Add virtual-kubelet binary to VIC ISO #7315

lcastellano · 2018-02-13T01:17:14Z

This PR comprises two changes:

The Makefile has been changed to add a new target appliance-vkube.iso, the path of the virtual-kubelet binary must be specified in an environment variable otherwise it defaults to: ./virtual-kubelet. The environment variable is called: VIRTUAL_KUBELET_PATH. The content of that variable is passed down to the script isos/appliance-extra.sh
the file iso/appliance-extra.sh has been added to retrieve the appropriate binary for virtual-kubelet.
If the path starts with gs:// it is assumed that the binary is found in: storage.googleapis.com and the latest version from the bucket is retrieved. Otherwise the path to the virtual-kubelet binary is considered a local path e.g. : /home/myname/golang/src/github.com/virtual-kubelet/virtual-kubelet/virtual-kubelet

The new iso/appliance-xtra.sh script has been developed to allow the addition of different binary files to the ISO, not specifically the virtual-kubelet binary.

zjs

I'd suggest reading the whole review before making any changes; a suggestion at the end may invalidate some preceding comments.

zjs · 2018-02-13T17:40:27Z

isos/appliance-xtra.sh

@@ -0,0 +1,185 @@
+#!/bin/bash
+# Copyright 2016 VMware, Inc. All Rights Reserved.


zjs · 2018-02-13T17:48:24Z

Makefile

@@ -420,6 +428,12 @@ $(appliance): isos/appliance.sh isos/appliance/* isos/vicadmin/** $(vicadmin) $(
 	@echo building VCH appliance ISO
 	@$(TIME) $< -p $(appliance-staging) -b $(BIN)

+.PHONY: $(appliance-vkube)
+# main appliance target + kubelet - depends on all top level component targets
+$(appliance-vkube): isos/appliance-xtra.sh isos/appliance/* isos/vicadmin/** $(vicadmin) $(vic-init) $(portlayerapi) $(docker-engine-api) $(appliance-staging) $(archive)


Is there a reason we use abbreviations like "xtra" and "vkube"? It doesn't seem like there's a lot of value to saving characters here. (And "vkube" is already a term VIO uses for something else.)

zjs · 2018-02-13T17:49:43Z

isos/appliance-xtra.sh

+            ;;
+
+        x)
+			# Required. Source of the xtra binary to add to the ISO


Whitespace seems to be inconsistent here and in several other places in this file.

zjs · 2018-02-13T17:51:56Z

isos/appliance-xtra.sh

+            ;;
+
+        o)
+			APPLIANCE_OUTNAME="$OPTARG"


Is there a reason this isn't required? It doesn't seem like we'd ever actually want to use the default value here. However, if we do want this to be optional, that should be documented in the usage string above.

zjs · 2018-02-13T17:54:20Z

isos/appliance-xtra.sh

+# limitations under the License.
+
+# Build the appliance filesystem ontop of the base
+set -x


This seems to be unnecessary; we do this on line 20.

zjs · 2018-02-13T17:59:11Z

isos/appliance-xtra.sh

+fi
+
+if [ -n "${XTRABIN}" ] && [ -z "${XTRABIN_FILENAME}" ]; then
+	echo "-f binary-filename  must be specificed when selecting -x"


This branch isn't reachable.

zjs · 2018-02-13T18:03:12Z

isos/appliance-xtra.sh

+echo RootFsDir $(rootfs_dir $PKGDIR)
+
+# sysctl
+cp ${DIR}/appliance/sysctl.conf $(rootfs_dir $PKGDIR)/etc/


Starting from here, this duplicates substantial non-trivial image authoring code from appliance.sh. We should have a single copy of this code so that we don't wind up with inconsistencies if they drift out of sync.

zjs · 2018-02-13T18:04:12Z

isos/appliance-xtra.sh

+# Below: the image authoring
+#################################################################
+
+echo RootFsDir $(rootfs_dir $PKGDIR)


This also seems like debugging logic which should either be removed or cleaned up.

That's how I started, George however prefers to have a different script for this appliance ISO. The belief is that the scripts will eventually diverge (e.g. a special port layer). Once we merge back the feature branch we will re-address the structure of these appliace-*.sh scripts. I will add a comment to the file as a reminder of this task.

zjs · 2018-02-13T18:04:41Z

isos/appliance-xtra.sh

+    APPLIANCE_NAME=appliance-xtra.iso
+fi
+
+echo Appliance name: $APPLIANCE_NAME


More debugging logic.

zjs · 2018-02-13T18:05:23Z

isos/appliance-xtra.sh

+
+echo Appliance name: $APPLIANCE_NAME
+
+if [ -n "${XTRABIN}" ]; then


We've already checked that this is set.

(However, one way to deduplicate the logic here with the logic in appliance.sh is to leave this check here, remove the check above, change the default name to appliance.iso, and rename this whole file to appliance.sh so that there's a single script that optionally includes an extra binary.)

sflxn · 2018-02-13T19:12:34Z

Why couldn't the new code in appliance_extra.sh have been included in appliance.sh instead? Why create a new script file? Maybe I didn't read it carefully, but it seems like that file is the same as appliance.sh with a few new arguments and code.

zjs

Just one minor comment. Other than that, this LGTM

zjs · 2018-02-14T23:09:20Z

isos/appliance-extra.sh

+    usage
+fi
+
+if [ -z "${EXTRABIN}" ] || [ -z "${EXTRABIN_FILENAME}" ] || [ -z "${APPLIANCE_OUTNAME}" ]; then


Is there a reason this is separate from the conditional above (and using a different style)? (Line 71)

* Dump dmesg if container rootfs blocks or fails mount (#7260) This is to enable bridging of the guest side state with the virtual hardware if we see issues such as disks not presenting on a pvscsi controller or a mount operation hanging. * updating priority definitions to include features (#7292) * Change default fellows for gandalf (#7310) * Avoid exposing test credentials in 12-01-Delete (#7306) To avoid exposing test credentials, use the established `Run VIC Machine Delete Command` keyword, which in turn calls a secret keyword. This changes the behavior of the test slightly: - It no longer checks for the absence of "delete failed" in output. - It will wait up to 30 seconds for the deletion to succeed. - It will clean up cert files at the end of the deletion. * Bug fix in API delete test: disable volume store cleanup (#7303) * Remove volume store cleanup before re-installing VIC appliance using existing volume stores * Cleanup dangling volume stores on test teardown * Add logging for image upload (#7296) * Reduce datastore searches during non-vSAN delete operations (#6951) * Optimize portlayer volume cache rebuild on startup (#7267) This commit modifies the portlayer volume cache's rebuildCache func to only process the volumes from the volume store that is currently being added to the cache. rebuildCache is invoked for every volume store during portlayer startup. Before this change, rebuildCache would process volumes from all volume stores in the cache every time a volume store was added. This led to unneeded extra computation which could slow down portlayer startup and overwhelm NFS endpoints if NFS volume stores are being used. Fixes #6991 * Added local ci testing target to makefile (#7170) Make testing locally as friction-free as possible by 1. Adding a makefile target 'local-ci-test' 2. Using TARGET_VCH added in VIC 1.3 to use an existing VCH 3. Using a custom script that doesn't utilize drone so that if the test fails and craters, we can still access the logs 4. Parameters can come from env vars, arguments, or secrets file Resolves #7162 * Added upload progress bar tracker for ISO images. (#7320) * Added upload progress bar tracker for ISO images. Removed concurrent upload since it doesn't make any significant performance imapact. When I tried to measure performance differene with and without concurrent uppload, the results were fluctuating in a wide range so no good measurement was possible. * Document the design for the vic-machine API (#6702) This document proposes a design for a comprehensive vic-machine API, the implementation of which will be tracked by #6116. Subsets of this API (tracked by #5721, #6123, and eventually others) will be implemented incrementally, and the design will be revised as those efforts progress to reflect changes to the long-term vision. * Remove superfluous calls to Set Test VCH Name (#7304) Several tests explicitly call the `Set Test VCH Name` keyword shortly after calling `Set Test Environment Variables`. This can lead to test failures when a VCH name collision occurs; subsequent tests which re-use the VCH name fail because there may be leftover certificates from the first VCH with that name. `Set Test Environment Variables` itself calls `Set Test VCH Name` and then cleans up old certificate directories. Therefore, the explicit calls to `Set Test VCH Name` are both superfluous and problematic. * Ensure that static ip worker is on the same nimbus pod as VC otherwise network connectivity not guaranteed (#7307) * [skip ci] Add ROBO test plans (#7297) This commit adds test plans for the ROBO support features in a new directory (Group19-ROBO) under manual test cases. The existing ROBO-SKU test has been moved into this directory. The test plans include tests for the container limit feature, placement without DRS, the license/ feature checks and WAN connectivity. Fixes #7294 * Add hosts to DVS within the test bed as well (#7326) * Setup updated for Longevity Tests (#7298) * Setup updated for Longevity Tests to run on 6.5U1 * [skip ci] Terminate gracefully to gather logs (#7331) * Terminate gracefully to gather logs * Remove extra whitespace * Increase timeout to 70 minutes * Increase ELM timeout to 70 minutes * Add repo to slack message since we have multiple repos reporting now (#7334) * Not sending user credentials with every request (#6382) * Add concurrent testing tool to tests folder (#6534) Adds a minimized test case for testing our core vSphere interactions at varying degrees of concurrency. This is intended to simplify debugging issues that are suspected to be platform problems, or API usage issues that are conceptually divorced from the VIC engine product code. * Refactor Install Harbor To Test Server keyword (#7335) The secret tag on the `Install Harbor To Test Server` makes it difficult to investigate failures when they occur. Only one out of 30+ lines actually uses secret information. Refactor the keyword to extract the secret information into its own keyword, allowing the tag to be applied in a more focused way. This is similar to the pattern used by keywords in `VCH-Util`. * Add ability to cache generated dependency. (#7340) * Add ability to cache generated dependency, so not much time wasted during the build process. * Added documentation to reflect necessary steps to leverage such improvements. * Ignore not-supported result from RetrieveUserGroups in VC 6.0 (#7328) * Move build time directives from title to body of PR (#7060) * Retry the harbor setup as well (#7336) * Skip non vSphere managed datastores when granting perms (#7346) * Fix volume leak on group 23 test (#7358) * Fix github status automation filtering (#7344) Adds filtering for the event source and consolidates remote API calls. Details the specific builds and their status for quick reference. * Drone 0.8 and HaaS updates (#7364) * Add tether.debug in integration test log bundle (#7422) * Update the gcs plugin (#7421) * [skip ci] Suggest subnet/gateway to static ip worker * Ensure that static ip worker is on the same nimbus pod as VC otherwise network connectivity not guaranteed (#7307) * Refactored some proxy code to reuse with wolfpack Refactored the system, volume, container, and stream swagger code into proxy code. 1) Moved the errors.go from backends to a new folder to be accessed by all folders outside of the backends folder. 2) Refactored Container proxy and moved from engine/backends to engine/proxy 3) Refactored Volume proxy and moved from engine/backends to engine/proxy 4) Refactored System proxy and moved from engine/backends to engine/proxy 5) Refactored Stream proxy and moved from engine/backends to engine/proxy 6) Adopted some common patterns in all the proxies 7) Moved common networking util calls to engine/networking 8) Fix up unit tests 9) Changed all "not yet implemented messages" 10) Updated robot scripts More refactoring will be needed to make these proxy less dependent on docker types and portlayer swagger types. Helps resolves #7210 and #7232 * Add virtual-kubelet binary to VIC ISO (#7315) * Start virtual-kubelet inside the VCH (#7369) * Fix value of the PORTLAYER_ADDR environment variable (#7400) * Use vic kubelet provider * Made modifications for virtual kubelet * Added admin log collection and fix env variable content (#7404) * Added most-vkubelet target (#7418)

Start virtual-kubelet inside the VCH (vmware#7369) Fix value of the PORTLAYER_ADDR environment variable (vmware#7400)

Start virtual-kubelet inside the VCH Fix value of the PORTLAYER_ADDR environment variable Added admin log collection and fix env variable content Added most-vkubelet target Build non-stripped binaries when env VIC_DEBUG_BUILD is set

lcastellano requested a review from zjs February 13, 2018 01:17

vmwclabot added the cla-not-required label Feb 13, 2018

zjs reviewed Feb 13, 2018

View reviewed changes

lcastellano force-pushed the iso branch from 3a9a9da to 7c0e289 Compare February 13, 2018 19:45

zjs reviewed Feb 14, 2018

View reviewed changes

Add virtual-kubelet binary to VIC ISO

e84930e

lcastellano force-pushed the iso branch from 7c0e289 to e84930e Compare February 15, 2018 20:36

zjs approved these changes Feb 16, 2018

View reviewed changes

sflxn approved these changes Feb 16, 2018

View reviewed changes

lcastellano merged commit 033771e into vmware:feature/wolfpack Feb 16, 2018

lcastellano deleted the iso branch February 20, 2018 17:39

sflxn pushed a commit to sflxn/vic that referenced this pull request Feb 28, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

c79e48a

sflxn pushed a commit to sflxn/vic that referenced this pull request Feb 28, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

1091754

sflxn pushed a commit to sflxn/vic that referenced this pull request Mar 1, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

235c8d7

sflxn pushed a commit to sflxn/vic that referenced this pull request Mar 1, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

9854bd7

sflxn pushed a commit that referenced this pull request Mar 2, 2018

Add virtual-kubelet binary to VIC ISO (#7315)

c763a92

sflxn pushed a commit to sflxn/vic that referenced this pull request May 2, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

261f0ac

sflxn pushed a commit to sflxn/vic that referenced this pull request Jun 27, 2018

Add virtual-kubelet binary to VIC ISO (vmware#7315)

937498f

Start virtual-kubelet inside the VCH (vmware#7369) Fix value of the PORTLAYER_ADDR environment variable (vmware#7400)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add virtual-kubelet binary to VIC ISO #7315

Add virtual-kubelet binary to VIC ISO #7315

lcastellano commented Feb 13, 2018 •

edited

Loading

zjs left a comment

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

lcastellano Feb 13, 2018

zjs Feb 13, 2018

zjs Feb 13, 2018

sflxn commented Feb 13, 2018

zjs left a comment

zjs Feb 14, 2018

		@@ -0,0 +1,185 @@
		#!/bin/bash
		# Copyright 2016 VMware, Inc. All Rights Reserved.


		echo Appliance name: $APPLIANCE_NAME

		if [ -n "${XTRABIN}" ]; then

Add virtual-kubelet binary to VIC ISO #7315

Add virtual-kubelet binary to VIC ISO #7315

Conversation

lcastellano commented Feb 13, 2018 • edited Loading

zjs left a 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

sflxn commented Feb 13, 2018

zjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcastellano commented Feb 13, 2018 •

edited

Loading