Add introspection service and a script to collect support bundle #8

liwenwu-amazon · 2017-11-30T19:27:17Z

Add following 2 introspection commands:

curl http://local:51678/v1/eni
cur http:/local:51678/v1/pod

Also add aws-cni-support.sh which will collect aws-vpc-cni-k8s cni related debugging information into a support bundle

rtripat · 2017-12-01T00:35:51Z

Can you make the introspection path as plural /v1/enis and /v1/pods
kubelet read-only port default is 10255 but it can be changed by users. take it as an input to the aws-cni-support.sh, if not provided default to 10255

dbenhur

I think you want to run gofmt on this.

dbenhur · 2017-12-01T00:42:36Z

ipamd/datastore/data_store.go

@@ -90,7 +107,7 @@ func NewDataStore() *DataStore {
 }

 // AddENI add eni to data store
-func (ds *DataStore) AddENI(eniID string, deviceNumber int, isPrimary bool) error {
+func (ds *DataStore) AddENI(eniID string, DeviceNumber int, IsPrimary bool) error {


Why are we Capitalizing function args?

will change it back to lowercase

dbenhur · 2017-12-01T01:04:55Z

ipamd/datastore/data_store.go

+	var eniInfos = ENIInfos{
+		TotalIPs:    ds.total,
+		AssignedIPs: ds.assigned,
+		ENIIPPools:  make(map[string]ENIIPPool),


make(map[string]ENIIPPool, len(ds.eniIPPools)) will give the right initial size and avoid reallocations as you copy.

done. thanks

dbenhur · 2017-12-01T01:12:34Z

ipamd/datastore/data_store.go

+	// AssignedIPv4Addresses is the number of ip addesses already been assigned
+	AssignedIPv4Addresses int
+	// IPv4Addresses shows whether each address is assigned
+	IPv4Addresses map[string]*AddressInfo


I presume these name changes are to export the struct members for visibility in the introspection package; why exclude the time fields then? Also the nice formatting alignment got messed up. Oh, and please consistently capitalize initialisms ENI,IP,etc in the comments.

dbenhur · 2017-12-01T01:20:04Z

ipamd/datastore/data_store.go

+	DeviceNumber int
+	// AssignedIPv4Addresses is the number of ip addesses already been assigned
+	AssignedIPv4Addresses int
+	// IPv4Addresses shows whether each address is assigned


Please document the key format

dbenhur · 2017-12-01T01:21:20Z

ipamd/datastore/data_store.go

+	// AssignedIPv4Addresses is the number of ip addesses already been assigned
+	AssignedIPv4Addresses int
+	// IPv4Addresses shows whether each address is assigned
+	IPv4Addresses map[string]*AddressInfo
 }

 // AddressInfo contains inforation about an IP
 type AddressInfo struct {
 	address        string


Please document the form of this string. Is it redundant with the key of the map to these structs?

The form of this string is the dot notation of IP address. Making address as part of AddressInfo struct can make AddressInfo re-used by other part of code,

The form should be documented in the code; suggest: "IP address strings must be in dot-decimal notation with no leading zeroes and no whitespace (eg: '10.1.0.253')"

Note: there was an attempt to standardize text representation via IETF RFC, though it was never formally accepted. As you can see from that doc, there is historical ambiguity in how to represent IP addresses as text.

dbenhur · 2017-12-01T01:25:49Z

ipamd/datastore/data_store.go

+	ds.lock.Lock()
+	defer ds.lock.Unlock()
+
+	var podInfos = make(map[string]PodIPInfo)


make(map[string]PodIPInfo, len(ds.podsIP))

dbenhur · 2017-12-01T01:35:06Z

ipamd/introspect.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		responseJSON, err := json.Marshal(ipam.dataStore.GetENIInfos())
+		if err != nil {
+			log.Error("Failed to introspect eni data: %v", err)


"Failed to marshal ENI data"

dbenhur · 2017-12-01T01:36:24Z

ipamd/introspect.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		responseJSON, err := json.Marshal(ipam.dataStore.GetPodInfos())
+		if err != nil {
+			log.Error("Failed to introspect pod data: %v", err)


dbenhur · 2017-12-01T02:01:36Z

pkg/utils/backoff.go

+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package utils


add a TODO to extract this to library (it's copied from ecs agent, yes?)

dbenhur · 2017-12-01T18:53:15Z

ipamd/introspect.go

@@ -122,7 +122,7 @@ func podV1RequestHandler(ipam *IPAMContext) func(http.ResponseWriter, *http.Requ
 	return func(w http.ResponseWriter, r *http.Request) {
 		responseJSON, err := json.Marshal(ipam.dataStore.GetPodInfos())
 		if err != nil {
-			log.Error("Failed to introspect pod data: %v", err)
+			log.Error("Failed to marshal POD data: %v", err)


"pod" is not an initialism

dbenhur · 2017-12-01T21:55:35Z

ipamd/datastore/data_store.go

@@ -270,7 +271,7 @@ func (ds *DataStore) FreeENI() (string, error) {
 	deletableENI := ds.getDeletableENI()
 	if deletableENI == nil {
 		log.Debugf("FreeENI: no deletable ENI")
-		return "", errors.New("free eni: none of enis can be deleted at this time")
+		return "", errors.New("free ENI: none of ENI can be deleted at this time")


"no ENI can be deleted "

dbenhur · 2017-12-01T21:56:08Z

ipamd/datastore/data_store.go

@@ -319,12 +320,12 @@ func (ds *DataStore) UnAssignPodIPv4Address(k8sPod *k8sapi.K8SPodInfo) (string,
 		}
 	}

-	log.Warnf("UnassignIPv4Address: Failed to find pod %s namespace %s Container %s using ip %s",
+	log.Warnf("UnassignIPv4Address: Failed to find pod %s namespace %s Container %s using IP %s",


lowercase Container

dbenhur · 2017-12-01T21:57:14Z

ipamd/datastore/data_store.go

 				k8sPod.IP, k8sPod.Name, k8sPod.Namespace, k8sPod.Container)
 			return ipAddr.IP, ipAddr.DeviceNumber, nil
 		}
 		//TODO handle this bug assert?, may need to add a counter here, if counter is too high, need to mark node as unhealthy...
 		// this is a bug that the caller invoke multiple times to assign(PodName/NameSpace -> a different IPaddress).
-		log.Errorf("AssignPodIPv4Address:  current ip %s is changed to ip %s for POD(name %s, namespace %s, container %s)",
+		log.Errorf("AssignPodIPv4Address:  current IP %s is changed to IP %s for POD(name %s, namespace %s, container %s)",


lowercase POD

dbenhur · 2017-12-01T22:00:04Z

ipamd/datastore/data_store.go


-// ENIIPPool contains ENI/IP Pool information
+// ENIIPPool contains ENI/IP Pool information. The capitalized variables are used for introspection.


s/The capitalized variables are used for introspection/Exported fields will be Marshaled for introspection/

dbenhur · 2017-12-01T22:00:33Z

ipamd/datastore/data_store.go

 }

-// AddressInfo contains inforation about an IP
+// AddressInfo contains inforation about an IP, The Capitalized variables are used for introspection.


Exported fields will be Marshaled for introspection

dbenhur · 2017-12-01T22:12:50Z

scripts/aws-cni-support.sh

+  echo "=============================================" >> ${LOG_DIR}/${ROUTE_OUTPUT}
+  echo "ip route show table $i" >> $LOG_DIR/$ROUTE_OUTPUT
+  ip route show table $i >> $LOG_DIR/$ROUTE_OUTPUT
+done


Why {0..32} instead of ip route show table all?

will use ip route show table all

dbenhur · 2017-12-01T22:19:32Z

scripts/aws-cni-support.sh

+LOG_DIR="/var/log/aws-routed-eni"
+
+# collecting L-IPAMD introspection data
+curl http://localhost:51678/v1/enis | python -m json.tool > ${LOG_DIR}/eni.output


Is the python module json.tool always going to be available or does this rely on some base image or distribution having made sure it's present already? Is this just for pretty-printing the output? Perhaps l-ipamd could pretty print by default using MarshalIndent?

ok, will remove python pretty print for now

dbenhur · 2017-12-01T22:24:40Z

scripts/aws-cni-support.sh

+curl http://localhost:51678/v1/pods | python -m json.tool > ${LOG_DIR}/pod.output
+
+# collecting kubeleet introspection data
+curl http://localhost:10255/pods | python -m json.tool > ${LOG_DIR}/kubelet.output


Raghav @rtripat asked for "kubelet read-only port default is 10255 but it can be changed by users. take it as an input to the aws-cni-support.sh, if not provided default to 10255"

samuelkarp · 2017-12-21T07:33:59Z

ipamd/introspect.go

+
+const (
+	// IntrospectionPort is the port for ipamd introspection
+	IntrospectionPort = 51678


Can you pick a different port than this? 51678 is used by the ECS agent and picking this port will prevent this plugin and ECS from ever being used together.

@samuelkarp thanks and have opened #19 for this.

# This is the 1st commit message: Add VlanId in the cmdAdd Result struct This VlanId will appear in the prevResult during cmdDel request Test prevResult contents CleanUp Pod Network using vlanId from prevResult in CNI itself No need to call ipamd Log formatting changes Added hostNetworking Setup test for pods using security groups revoke unnecessary test agent image changes Revoke unnecessary changes remove focussed test set replica count to total number of branch interface Fix replica count # This is the commit message aws#2: Updated cleanUpPodENI method # This is the commit message aws#3: Skip processing Delete request if prevResult is nil Add Logging vlanId to ipamd # This is the commit message aws#4: Add support to test with containerd nodegroup in pod-eni test # This is the commit message aws#5: Add check for empty Netns() in cni # This is the commit message aws#6: Manifests and Readme updates (aws#1732) * Manifests and Readme updates * update manifest.jsonnet # This is the commit message aws#7: Readme updates (aws#1735) # This is the commit message aws#8: Updates to troubleshooting doc (aws#1737) * Updates to troubleshooting doc * updates to troubleshooting doc # This is the commit message aws#9: imdsv2 changes (aws#1743) # This is the commit message aws#10: fix flaky canary test (aws#1742) # This is the commit message aws#11: add CODEOWNERS (aws#1747)

liwenwu-amazon requested a review from a team November 30, 2017 19:27

dbenhur suggested changes Dec 1, 2017

View reviewed changes

dbenhur reviewed Dec 1, 2017

View reviewed changes

dbenhur approved these changes Dec 1, 2017

View reviewed changes

Add introspection service and add a script to collect support bundle

c80fec1

liwenwu-amazon force-pushed the introspection-nov27 branch from 8302c7c to c80fec1 Compare December 1, 2017 22:48

liwenwu-amazon merged commit ad7f406 into aws:master Dec 1, 2017

samuelkarp reviewed Dec 21, 2017

View reviewed changes

liwenwu-amazon mentioned this pull request Dec 21, 2017

Using different port for introspection service #19

Closed


		// ENIIPPool contains ENI/IP Pool information
		// ENIIPPool contains ENI/IP Pool information. The capitalized variables are used for introspection.

Add introspection service and a script to collect support bundle #8

Add introspection service and a script to collect support bundle #8

Conversation

liwenwu-amazon commented Nov 30, 2017

rtripat commented Dec 1, 2017

dbenhur 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment