[Power Support] Enable weaveworks scope on Power. #3231

meghalidhoble · 2018-06-20T07:26:24Z

Observed this particular test was failing on ppc64le architecture. Added the test failure log below,
walker_linux_test.go:97:

                +++ have
                @@ -32,7 +32,7 @@
                   Cmdline: (string) (len=15) "curl google.com",
                   Threads: (int) 1,
                   Jiffies: (uint64) 0,
                -  RSSBytes: (uint64) 8192,
                +  RSSBytes: (uint64) 131072,
                   RSSBytesLimit: (uint64) 2048,
                   OpenFilesCount: (int) 3,
                   OpenFilesLimit: (uint64) 32768,

Hence modified the value for RSSBytes to accept variable value which is computed based on pageSize instead of hard-coded values, as suggested by Adam Harrison over slack channel.

I have verified this patch on x86 setup as well and all the tests pass there. Please provide your comments/suggestions on this.
Thanks,
Meghali

meghalidhoble · 2018-06-20T08:29:16Z

There are some build changes as well, which should fix these error. Another PR is in-progress for the build fixes.

bboreham · 2018-06-20T11:02:01Z

Note there is a lint failure in this PR.

meghalidhoble · 2018-06-20T12:19:55Z

Yes, @bboreham , I am looking into this. I am expecting my build-changes should get this resolved.

probe/process/walker_linux_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/weaveworks/common/test"
 	"github.com/weaveworks/common/test/fs"
 	"github.com/weaveworks/scope/probe/process"
+	"os"


bboreham · 2018-06-20T12:24:14Z

Not sure we're talking about the same thing. The error is:

probe/process/walker_linux_test.go: run gofmt -s -w probe/process/walker_linux_test.go

satyamz · 2018-06-21T06:55:14Z

@meghalidhoble Please run make lint locally to verify linting issues.

client/Dockerfile

@@ -1,4 +1,4 @@
-FROM node:8.4.0
+FROM node:8.11


probe/endpoint/dns_snooper_linux_ppc64le.go

@@ -0,0 +1,284 @@
+package endpoint


meghalidhoble · 2018-06-21T09:24:22Z

Hi @satyamz , @bboreham,
I have observed that "make lint" is failing on my platform (ppc64le) without the test-patch as well.
I had tested using "make tests" and "make client-test", and both of them worked fine for me.
I need to debug this issue further, will get back to you on this.

bboreham · 2018-06-21T11:35:09Z

Please run gofmt -s -w probe/process/walker_linux_test.go then update the PR. This should clear the lint error.

meghalidhoble · 2018-06-21T12:33:43Z

Thanks @bboreham , your suggestion helped to resolve the failure.

backend/Dockerfile

+	go install -race -tags netgo std; \
+   fi
+RUN export arch_val="$(dpkg --print-architecture)"; \
+   if [ "$arch_val" != "ppc64el" ]; then \


backend/Dockerfile

@@ -1,14 +1,32 @@
-FROM golang:1.10.2-stretch
+FROM golang:1.10.2


bboreham · 2018-06-22T15:52:00Z

Please put into each commit message a description of the changes.
Some tips here
"code changes made" is not a useful description.

meghalidhoble · 2018-06-28T12:46:26Z

Hi @bboreham, @satyamz ,
Could you please provide your comments on the latest code changes I submitted?
I have ran the weave/scope docker image and the verified that the UI comes up. What more testing I can do around this, so as to make sure the changes works fine for the functionality?
Thanks.

abdasgupta · 2018-07-05T07:11:06Z

Hi @bboreham, @satyamz ,
Can we please merge this PR if no more modification is needed?

bboreham

This is much cleaner; I have some queries in the detail.
More generally, we still need to have every change described in commit messages - people will look back in a year and want to know why a line changed.

probe/endpoint/dns_snooper.go

@@ -1,3 +1,7 @@
+// +build linux,amd64 ppc64le


backend/Dockerfile

-RUN apt-get update && \
+RUN set -eux; \
+   export arch_val="$(dpkg --print-architecture)"; \
+   if [ "$arch_val" != "ppc64el" ]; then \


backend/Dockerfile

-    chmod +x shfmt && \
-    mv shfmt /usr/bin
+   export arch_val="$(dpkg --print-architecture)"; \
+   if [ "$arch_val" != "ppc64el" ]; then \


backend/Dockerfile

+     chmod +x shfmt && \
+     mv shfmt /usr/bin; \
+   else \
+     go get -u mvdan.cc/sh/cmd/shfmt && \


probe/endpoint/dns_snooper.go

@@ -1,4 +1,5 @@
-// +build linux,amd64 ppc64le
+// +build linux
+// +build amd64 ppc64le


backend/Dockerfile file - Minor if conditional changes to have cross-compile on amd64 only. Also removed the sh-lint support (shfmt installation) for ppc64le completely, as the older version of shfmt is not supported for ppc and newer version doesn't go well with application. tools/lint file - Added conditional to not run "sh lint" (make lint) for ppc64le. Fixes PR weaveworks#3231

…syntax corrections. Hence pushing the change according to the shfmt run from x86 setup. This change was to skip the sh-lint run on powerpc. PR weaveworks#3231

PR weaveworks#3231

meghalidhoble · 2018-07-10T11:58:39Z

Hi @bboreham ,
I have addressed most of your comments and have pushed a few new code changes, Could you please take a look and let me know if any comments/suggestions?

bboreham · 2018-07-10T12:27:17Z

@meghalidhoble please can you squash the commits into a smaller number so we don't have dead-ends and back-tracking in the history. Leave each commit explaining what it does.

…ppc64le) 1)backend/Dockerfile 2) probe/endpoint/dns_snooper.go 3) client/Dockerfile 4) docker/Dockerfile.cloud-agent 5) probe/process/walker_linux_test.go & 6) tools/lint 1)'backend/Dockerfile' : Conditional added so that the cross-compiling should be done on amd64. Also removed support for sh-lint for ppc64le for now. As the version for shfmt mentioned in the dockerfile is not available for ppc64le and the later version does't work fine with existing application. 2)'probe/endpoint/dns_snooper.go' : Renamed this file so as to reuse for ppc64le and added a build-constraint. Now this file will be build for amd64 on linux and ppc64le on linux. 3)'client/Dockerfile' : Modified the version of the base image for node from 8.4.0 to 8.11, as this version supports multiarch. 4)'docker/Dockerfile.cloud-agent' : Modified the version of the base image for golang from 1.10.2-strech to 1.10.2, which supports multiarch. 5) 'probe/process/walker_linux_test.go' : Test fixed to run for ppc64le, modified the code to accept RSSBytes based on pageSize value per architecture, instead of hard-coded values. 6)'tools/lint' : Modified the file to skip the sh-lint implementation for ppc64le. PR weaveworks#3231

meghalidhoble · 2018-07-11T10:58:01Z

Hi @bboreham , squashed all the commits to a single commit, with all the implementation details for each file being modified. Please take a look.

bboreham · 2018-07-16T09:32:54Z

Sadly, it is inappropriate to directly modify anything in the tools directory because it is a vendored copy of https://github.com/weaveworks/build-tools. Need to modify it upstream and then bring that commit in.

I'm sure this is maddening.

I might just change it to do nothing if it can't find shfmt, as is done for shellcheck, rather than have an architecture-specific test.

…heck. Why the change : Need to skip the shfmt validation, for ppc64le as shfmt specified version is not available. Hence added a check to run shfmt if the binary found, else skip the validation completely. PR weaveworks#3231

Why the change : make lint fails while sh-lint is being executed. PR weaveworks#3231

Why : Corrected comment and re-formatted as per guidelines PR weaveworks#3231

meghalidhoble · 2018-07-18T07:04:39Z

Hi @bboreham ,
I have addressed the concern raised for tools directory changes, hence I have removed the architecture specific changes. Sorry for multiple commits, I will squash those commits and will combine to a single commit. Let me know if these changes look ok.

bboreham · 2018-07-19T13:21:19Z

Multiple commits is fine as long as they all form a progression and are all self-explanatory.

However the issue is that to change a file under the tools directory in this repo you need to change it in https://github.com/weaveworks/build-tools first and then do a git subtree to update the directory in this repo, as described in tools/README.md.

meghalidhoble · 2018-08-02T11:06:46Z

Hi @bboreham , I have raised a pull request at build-tools repo with the lint changes, once those changes are approved, I will run git subtree to update the directory in this repo.

…ppc64le) 1)backend/Dockerfile 2) probe/endpoint/dns_snooper.go 3) client/Dockerfile 4) docker/Dockerfile.cloud-agent 5) probe/process/walker_linux_test.go & 6) tools/lint 1)'backend/Dockerfile' : Conditional added so that the cross-compiling should be done on amd64. Also removed support for sh-lint for ppc64le for now. As the version for shfmt mentioned in the dockerfile is not available for ppc64le and the later version does't work fine with existing application. 2)'probe/endpoint/dns_snooper.go' : Renamed this file so as to reuse for ppc64le and added a build-constraint. Now this file will be build for amd64 on linux and ppc64le on linux. 3)'client/Dockerfile' : Modified the version of the base image for node from 8.4.0 to 8.11, as this version supports multiarch. 4)'docker/Dockerfile.cloud-agent' : Modified the version of the base image for golang from 1.10.2-strech to 1.10.2, which supports multiarch. 5) 'probe/process/walker_linux_test.go' : Test fixed to run for ppc64le, modified the code to accept RSSBytes based on pageSize value per architecture, instead of hard-coded values. 6)'tools/lint' : Modified the file to skip the sh-lint implementation for ppc64le. PR weaveworks#3231

bboreham · 2018-08-24T14:28:10Z

Thanks!

bboreham reviewed Jun 20, 2018

View reviewed changes

probe/process/walker_linux_test.go Outdated

@@ -8,6 +8,7 @@ import (

"github.com/weaveworks/common/test"

"github.com/weaveworks/common/test/fs"

"github.com/weaveworks/scope/probe/process"

"os"

This comment was marked as abuse.

Sign in to view

satyamz reviewed Jun 21, 2018

View reviewed changes

bboreham reviewed Jun 22, 2018

View reviewed changes

backend/Dockerfile Outdated

go install -race -tags netgo std; \

fi

RUN export arch_val="$(dpkg --print-architecture)"; \

if [ "$arch_val" != "ppc64el" ]; then \

This comment was marked as abuse.

Sign in to view

bboreham reviewed Jun 22, 2018

View reviewed changes

backend/Dockerfile

@@ -1,14 +1,32 @@

FROM golang:1.10.2-stretch

FROM golang:1.10.2

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

meghalidhoble changed the title ~~Modified the test "walker_linux_test.go", so as to be able to run on ppc64le.~~ [Power Support] Enable weaveworks scope on Power. Jun 26, 2018

bboreham reviewed Jul 5, 2018

View reviewed changes

meghalidhoble commented Jul 6, 2018

View reviewed changes

probe/endpoint/dns_snooper.go Outdated

@@ -1,4 +1,5 @@

// +build linux,amd64 ppc64le

// +build linux

// +build amd64 ppc64le

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

meghalidhoble added a commit to meghalidhoble/scope that referenced this pull request Jul 10, 2018

Change - Another shfmt formatting correction.

6249f77

PR weaveworks#3231

meghalidhoble force-pushed the master branch from 6249f77 to 84884e1 Compare July 11, 2018 10:38

meghalidhoble added a commit to meghalidhoble/scope that referenced this pull request Jul 17, 2018

Change : tools/lint file for formatting changes.

234c841

Why the change : make lint fails while sh-lint is being executed. PR weaveworks#3231

meghalidhoble added a commit to meghalidhoble/scope that referenced this pull request Jul 18, 2018

Change : tools/lint

7bcb766

Why : Corrected comment and re-formatted as per guidelines PR weaveworks#3231

meghalidhoble force-pushed the master branch from 7bcb766 to 84884e1 Compare August 10, 2018 05:24

bboreham mentioned this pull request Aug 10, 2018

Update the 'tools' subdirectory #3312

Merged

meghalidhoble force-pushed the master branch from 06a9442 to 625998b Compare August 13, 2018 07:18

bboreham approved these changes Aug 24, 2018

View reviewed changes

bboreham merged commit 321c570 into weaveworks:master Aug 24, 2018

bboreham mentioned this pull request Aug 24, 2018

CI test broken by Alpine upgrade #3331

Closed

fbarl mentioned this pull request Sep 18, 2019

Update javascript libraries in various places #3685

Merged

		@@ -1,14 +1,32 @@
		FROM golang:1.10.2-stretch
		FROM golang:1.10.2

[Power Support] Enable weaveworks scope on Power. #3231

[Power Support] Enable weaveworks scope on Power. #3231

Conversation

meghalidhoble commented Jun 20, 2018

meghalidhoble commented Jun 20, 2018

bboreham commented Jun 20, 2018

meghalidhoble commented Jun 20, 2018

This comment was marked as abuse.

bboreham commented Jun 20, 2018

satyamz commented Jun 21, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

meghalidhoble commented Jun 21, 2018

bboreham commented Jun 21, 2018

meghalidhoble commented Jun 21, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Jun 22, 2018

meghalidhoble commented Jun 28, 2018

abdasgupta commented Jul 5, 2018

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

meghalidhoble commented Jul 10, 2018

bboreham commented Jul 10, 2018

meghalidhoble commented Jul 11, 2018

bboreham commented Jul 16, 2018

meghalidhoble commented Jul 18, 2018

bboreham commented Jul 19, 2018

meghalidhoble commented Aug 2, 2018

bboreham commented Aug 24, 2018