Fix/ub 624 flex should not write log into flex driver director

[hfeish]

Handle flex logrotate internal
configurable for FLEX-LOG-DIR in ubiquity-configmap.yml and ubiquity_installer.conf, default is /var/log, customer need to make sure Flex-Log-dir exist in the minors
configurable for FLEX-LOG-ROTATE-MAXSIZE in ubiquity-configmap.yml, default is 50MB

How to test:
1. check the default log path: /var/log(exist in all flex pods, and nodes), logs will rotate if > 50 MB, will keep 5 compressed log file
2. can change log path to example /var/log/ubiquity in ubiquity_install.conf and configmap, need to make sure this path existed(in all nodes, also for controller-manager-pod) before install
3. can change log rotate size from 50MB to others in ubiquity-configmap
4. flex binary will create the log dir (ex: /var/log/ubiquity) if it not exist
5. flex logs not sit on k8s plugin directory

---

This change is 

[shay-berman]

Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

Dockerfile.Flex, line 11 at r1 (raw file):

FROM alpine:latest
RUN apk --no-cache add ca-certificates
RUN apk --update add logrotate

Fei, don't forget to remove also the logrotate apk add, since we don't need it any more.

---

Comments from Reviewable

[hfeish]

remove also the logrotate apk add

[shay-berman]

Review status: 0 of 9 files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.

---

deploy/k8s_deployments/setup_flex.sh, line 9 at r2 (raw file):

#    /usr/libexec/kubernetes/kubelet-plugins/volume/exec/ibm~ubiquity-k8s-flex
# 2. Run tail -f on the flex log file, so it will be visible via kubectl logs <flex Pod>
# 3. Start infinite loop with logrotate every 24 hours on the host flex log file

bring this line back, because we still do infinit loop, but now only for tailing the log

---

deploy/k8s_deployments/setup_flex.sh, line 159 at r2 (raw file):

echo "Running in the background tail -F <flex log>, so the log will be visible though kubectl logs <flex POD>"
echo "[`date`] Start to run in background #>"
echo "tail -F ${FLEX_LOG_DIR}/ubiquity-k8s-flex.log"

please don't hardcoded the flex log file, just use ${DRIVER}

---

deploy/k8s_deployments/setup_flex.sh, line 161 at r2 (raw file):

echo "tail -F ${FLEX_LOG_DIR}/ubiquity-k8s-flex.log"
echo "-----------------------------------------------"
tail -F ${FLEX_LOG_DIR}/ubiquity-k8s-flex.log &

same here,, just use ${DRIVER} instead of ubiqutiy-k8s-flex.log

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 32 at r2 (raw file):

   # The following keys are used by ubiquity, ubiquity-k8s-provisioner deployments, And ubiquity-k8s-flex daemon-set
   # ----------------------------------------------------------------------------------------------------------------------------
   # Flex log path. Allow to configure it, just make sure the the new path exist on all the minons and update the hostpath in the daemonset

"daemonset" -> "Flex daemonset"

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 33 at r2 (raw file):

   # ----------------------------------------------------------------------------------------------------------------------------
   # Flex log path. Allow to configure it, just make sure the the new path exist on all the minons and update the hostpath in the daemonset
   FLEX_LOG_DIR: "FLEX_LOG_DIR_VALUE"

keep the convention, the key name should be with - and not with _. please fix it FLEX-LOG-DIR

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 35 at r2 (raw file):

   FLEX_LOG_DIR: "FLEX_LOG_DIR_VALUE"

   #Maxlog size(MB) for rotate

missing space after "#" sign

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 36 at r2 (raw file):

   #Maxlog size(MB) for rotate
   FLEX_LOG_ROTATE_MAXSIZE: "50"

FLEX-LOG-ROTATE-MAXSIZE

also please make sure you update Dima about these new variables so he will update the UG according.

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity_installer.conf, line 48 at r2 (raw file):

# Parameters in ubiquity-configmap.yml that impact on "ubiquity" and "ubiquity-k8s-provisioner" deployments, And "ubiquity-k8s-flex" daemonset
#-----------------------------------------------------------------
# Flex log path. Allow to configure it, just make sure the the new path exist on all the minons and update the hostpath in the daemonset

Flex log directory. if you change the default, then make the new path exist on all the nodes and update the Flex daemonset hostpath according.

Note: validate string with Dima

---

scripts/installer-for-ibm-storage-enabler-for-containers/yamls/ubiquity-k8s-flex-daemonset.yml, line 39 at r2 (raw file):

              configMapKeyRef:
                name: ubiquity-configmap
                key: FLEX_LOG_DIR

don't forget to change the key to FLEX-LOG-DIR also below

---

scripts/installer-for-ibm-storage-enabler-for-containers/yamls/ubiquity-k8s-flex-daemonset.yml, line 104 at r2 (raw file):

      - name: flex-log-dir
        hostPath:
          path: FLEX_LOG_DIR_VALUE  # This directory must exist on the host

append to the note : if not the flex log will not be persistent and the flex pod will not be able to tail the log.

---

utils/utils.go, line 14 at r2 (raw file):

	config := resources.UbiquityPluginConfig{}
	config.LogLevel = os.Getenv("LOG_LEVEL")
	LogRotateMaxSize, err := strconv.Atoi(os.Getenv("FLEX_LOG_ROTATE_MAXSIZE"))

BUG
you should have a default in case the variable is not define.
its not enough to count on the default in the flex init script

---

Comments from Reviewable

[hfeish]

Review status: 0 of 9 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

deploy/k8s_deployments/setup_flex.sh, line 9 at r2 (raw file):

Previously, shay-berman wrote…

bring this line back, because we still do infinit loop, but now only for tailing the log

bring back

---

deploy/k8s_deployments/setup_flex.sh, line 159 at r2 (raw file):

Previously, shay-berman wrote…

please don't hardcoded the flex log file, just use ${DRIVER}

Done

---

deploy/k8s_deployments/setup_flex.sh, line 161 at r2 (raw file):

Previously, shay-berman wrote…

same here,, just use ${DRIVER} instead of ubiqutiy-k8s-flex.log

Done

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 32 at r2 (raw file):

Previously, shay-berman wrote…

"daemonset" -> "Flex daemonset"

Done

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 33 at r2 (raw file):

Previously, shay-berman wrote…

keep the convention, the key name should be with - and not with _. please fix it FLEX-LOG-DIR

Done

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 35 at r2 (raw file):

Previously, shay-berman wrote…

missing space after "#" sign

Done

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity-configmap.yml, line 36 at r2 (raw file):

Previously, shay-berman wrote…

FLEX-LOG-ROTATE-MAXSIZE

also please make sure you update Dima about these new variables so he will update the UG according.

Done, will info Dima

---

Comments from Reviewable

[hfeish]

Review status: 0 of 9 files reviewed at latest revision, 12 unresolved discussions, some commit checks broke.

---

scripts/installer-for-ibm-storage-enabler-for-containers/ubiquity_installer.conf, line 48 at r2 (raw file):

Previously, shay-berman wrote…

Flex log directory. if you change the default, then make the new path exist on all the nodes and update the Flex daemonset hostpath according.

Note: validate string with Dima

Done

---

Comments from Reviewable

[shay-berman]

@hfeish please do NOT merge to dev yet. We may postponed this fix to the next Ubiquity release.

[shay-berman]

I reviewed it now, please apply the comments and then you can merge it.

---

Review status: 0 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Dockerfile.Flex, line 11 at r1 (raw file):

Previously, shay-berman wrote…

Fei, don't forget to remove also the logrotate apk add, since we don't need it any more.

ok i see you removed it

---

glide.yaml, line 12 at r3 (raw file):

  - lib
- package: github.com/IBM/ubiquity
  version: fix/UB-624_Flex_should_not_write_log_into_flex_driver_directory

please don't merge this version to dev.

---

deploy/k8s_deployments/setup_flex.sh, line 82 at r3 (raw file):

}

function create_flex_log_dir()

is this needed? does the flex it self create the dir if not exist?

---

scripts/installer-for-ibm-storage-enabler-for-containers/yamls/ubiquity-k8s-flex-daemonset.yml, line 39 at r2 (raw file):

Previously, shay-berman wrote…

don't forget to change the key to FLEX-LOG-DIR also below

thanks for fixing

---

utils/utils.go, line 14 at r2 (raw file):

Previously, shay-berman wrote…

BUG
you should have a default in case the variable is not define.
its not enough to count on the default in the flex init script

ok
I see that you fixed that
please try to add UT for it as well.

---

Comments from Reviewable

[hfeish]

Review status: 0 of 10 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

glide.yaml, line 12 at r3 (raw file):

Previously, shay-berman wrote…

please don't merge this version to dev.

Sure, I will change it to dev while merging to dev

---

deploy/k8s_deployments/setup_flex.sh, line 82 at r3 (raw file):

Previously, shay-berman wrote…

is this needed? does the flex it self create the dir if not exist?

You're right, we don't need this

---

utils/utils.go, line 14 at r2 (raw file):

Previously, shay-berman wrote…

ok
I see that you fixed that
please try to add UT for it as well.

Any suggestion here? you mean we need to add UT for "func LoadConfig"?

---

Comments from Reviewable

[shay-berman]

Review status: 0 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

utils/utils.go, line 14 at r2 (raw file):

Previously, hfeish (Fei Huang) wrote…

Any suggestion here? you mean we need to add UT for "func LoadConfig"?

you don't need to mock something here
just create UT that call this function while you change the envs before calling it, and make sure it do the Loadconfiguration well.

---

Comments from Reviewable

[hfeish]

Review status: 0 of 11 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

utils/utils.go, line 14 at r2 (raw file):

Previously, shay-berman wrote…

you don't need to mock something here
just create UT that call this function while you change the envs before calling it, and make sure it do the Loadconfiguration well.

Add utils_test.go for this UT

---

Comments from Reviewable

[shay-berman]

Review status: 0 of 11 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

utils/utils_test.go, line 4 at r4 (raw file):

import (
	"testing"

why you are not doing it with Ginkgo? like we do for all the UTs?
with Describe \ BeforeEach \ Context and then It() testing?

any reason why you choose to do it different?

its a bit hard to understand what you want to test in tests1 and tests1

---

utils/utils_test.go, line 92 at r4 (raw file):

	ubiquityConfig, err := k8sutils.LoadConfig()
	if err != nil {
		fmt.Println("LoadConfig failed: ",err)

why you just print error? don't you need to fail here?

---

Comments from Reviewable

[hfeish]

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

utils/utils_test.go, line 4 at r4 (raw file):

Previously, shay-berman wrote…

why you are not doing it with Ginkgo? like we do for all the UTs?
with Describe \ BeforeEach \ Context and then It() testing?

any reason why you choose to do it different?

its a bit hard to understand what you want to test in tests1 and tests1

I use it because we need a lot data to set in the env to test, so I use the data table drive test.
In test1, set the correct env value and after LoadConfig it will get the same value
In test2, since in LoadConfig FLEX_LOG_ROTATE_MAXSIZE and SCBE_SKIP_RESCAN_ISCSI have default value, so test it here.

---

utils/utils_test.go, line 92 at r4 (raw file):

Previously, shay-berman wrote…

why you just print error? don't you need to fail here?

Add t.Fail() here

---

Comments from Reviewable

[shay-berman]

Review status: 0 of 11 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

utils/utils_test.go, line 4 at r4 (raw file):

Previously, hfeish (Fei Huang) wrote…

I use it because we need a lot data to set in the env to test, so I use the data table drive test.
In test1, set the correct env value and after LoadConfig it will get the same value
In test2, since in LoadConfig FLEX_LOG_ROTATE_MAXSIZE and SCBE_SKIP_RESCAN_ISCSI have default value, so test it here.

but u can still use data set in ginkgo test framework and the make it more standard lile all other test structure and then u can use assert insted of doing if else stuff.

so just wonder if there is real reason u used testing instead of ginkgo?
btw r u sure that by running scripts/run_unit script it will trigger this test?

---

Comments from Reviewable

[hfeish]

Review status: 0 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

utils/utils_test.go, line 4 at r4 (raw file):

Previously, shay-berman wrote…

but u can still use data set in ginkgo test framework and the make it more standard lile all other test structure and then u can use assert insted of doing if else stuff.

so just wonder if there is real reason u used testing instead of ginkgo?
btw r u sure that by running scripts/run_unit script it will trigger this test?

Yes, you are right, change it to ginkgo, please review

---

Comments from Reviewable

[shay-berman]

Review my last comments
anyway it

---

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

glide.yaml, line 13 at r5 (raw file):

- package: github.com/IBM/ubiquity
  version: fix/UB-624_Flex_should_not_write_log_into_flex_driver_directory
  subpackages:

make sure to set it to dev branch before merge to dev.

---

utils/utils_test.go, line 4 at r4 (raw file):

Previously, hfeish (Fei Huang) wrote…

Yes, you are right, change it to ginkgo, please review

much better, thanks!

small improvement (its not a must) - but I would move the tests1 var into the first It and tests2 into second It.
but anyway its good enough.

---

Comments from Reviewable