Skip to content

Fix loop pointer references #1982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,12 @@ jobs:
- checkout
- setup_remote_docker
- install-go
- run: go get -u -v golang.org/x/lint/golint
- run: sudo pip install black aiohttp
- run:
name: Install Linting Tools
command: |
go get -u -v golang.org/x/lint/golint
go get -u -v github.com/kyoh86/looppointer/cmd/looppointer
sudo pip install black aiohttp
- run:
name: Lint
command: make lint
Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ registry-clean-aws:

tools:
@go get -u -v golang.org/x/lint/golint
@go get -u -v github.com/kyoh86/looppointer/cmd/looppointer
@go get -u -v github.com/VojtechVitek/rerun/cmd/rerun
@go get -u -v github.com/go-delve/delve/cmd/dlv
@if [[ "$$OSTYPE" == "darwin"* ]]; then brew install parallel; elif [[ "$$OSTYPE" == "linux"* ]]; then sudo apt-get install -y parallel; else echo "your operating system is not supported"; fi
Expand Down
11 changes: 11 additions & 0 deletions build/lint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ if ! command -v golint >/dev/null 2>&1; then
exit 1
fi

if ! command -v looppointer >/dev/null 2>&1; then
echo "looppointer must be installed"
exit 1
fi

if ! command -v gofmt >/dev/null 2>&1; then
echo "gofmt must be installed"
exit 1
Expand All @@ -52,6 +57,12 @@ if [[ $output ]]; then
exit 1
fi

output=$(looppointer "$ROOT/...")
if [[ $output ]]; then
echo "$output"
exit 1
fi

output=$(gofmt -s -l "$ROOT")
if [[ $output ]]; then
echo "go files not properly formatted:"
Expand Down
16 changes: 8 additions & 8 deletions pkg/lib/archive/archiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,29 @@ func archive(input *Input, arc archiver) (strset.Set, error) {
addedPaths := strset.New()
var err error

for _, byteInput := range input.Bytes {
err = addBytesToArchive(&byteInput, input, arc, addedPaths)
for i := range input.Bytes {
err = addBytesToArchive(&input.Bytes[i], input, arc, addedPaths)
if err != nil {
return nil, err
}
}

for _, fileInput := range input.Files {
err = addFileToArchive(&fileInput, input, arc, addedPaths)
for i := range input.Files {
err = addFileToArchive(&input.Files[i], input, arc, addedPaths)
if err != nil {
return nil, err
}
}

for _, dirInput := range input.Dirs {
err = addDirToArchive(&dirInput, input, arc, addedPaths)
for i := range input.Dirs {
err = addDirToArchive(&input.Dirs[i], input, arc, addedPaths)
if err != nil {
return nil, err
}
}

for _, fileListInput := range input.FileLists {
err = addFileListToArchive(&fileListInput, input, arc, addedPaths)
for i := range input.FileLists {
err = addFileListToArchive(&input.FileLists[i], input, arc, addedPaths)
if err != nil {
return nil, err
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/lib/k8s/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ func IsPodReady(pod *kcore.Pod) bool {
}

func GetPodReadyTime(pod *kcore.Pod) *time.Time {
for _, condition := range pod.Status.Conditions {
for i := range pod.Status.Conditions {
condition := pod.Status.Conditions[i]

if condition.Type == "Ready" && condition.Status == kcore.ConditionTrue {
if condition.LastTransitionTime.Time.IsZero() {
return nil
Expand Down
8 changes: 6 additions & 2 deletions pkg/operator/endpoints/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ func getNodeInfos() ([]schema.NodeInfo, int, error) {
nodeInfoMap := make(map[string]*schema.NodeInfo, len(nodes)) // node name -> info
spotPriceCache := make(map[string]float64) // instance type -> spot price

for _, node := range nodes {
for i := range nodes {
node := nodes[i]

instanceType := node.Labels["beta.kubernetes.io/instance-type"]
nodeGroupName := node.Labels["alpha.eksctl.io/nodegroup-name"]
isSpot := strings.Contains(strings.ToLower(node.Labels["lifecycle"]), "spot")
Expand Down Expand Up @@ -128,7 +130,9 @@ func getNodeInfos() ([]schema.NodeInfo, int, error) {

var numPendingReplicas int

for _, pod := range pods {
for i := range pods {
pod := pods[i]

_, isAPIPod := pod.Labels["apiName"]

if pod.Spec.NodeName == "" && isAPIPod {
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ func main() {
exit.Error(errors.Wrap(err, "init"))
}

for _, deployment := range deployments {
for i := range deployments {
deployment := deployments[i]
apiKind := deployment.Labels["apiKind"]
if userconfig.KindFromString(apiKind) == userconfig.RealtimeAPIKind ||
userconfig.KindFromString(apiKind) == userconfig.AsyncAPIKind {
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/resources/asyncapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ func GetAllAPIs(pods []kcore.Pod, deployments []kapps.Deployment) ([]schema.APIR

realtimeAPIs := make([]schema.APIResponse, len(apis))

for i, api := range apis {
for i := range apis {
api := apis[i]
endpoint, err := operator.APIEndpoint(&api)
if err != nil {
return nil, err
Expand Down
4 changes: 3 additions & 1 deletion pkg/operator/resources/asyncapi/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ func getReplicaCounts(deployment *kapps.Deployment, pods []kcore.Pod) status.Rep
counts := status.ReplicaCounts{}
counts.Requested = *deployment.Spec.Replicas

for _, pod := range pods {
for i := range pods {
pod := pods[i]

if pod.Labels["apiName"] != deployment.Labels["apiName"] {
continue
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/operator/resources/job/batchapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
batchAPIsMap := map[string]*schema.APIResponse{}

jobIDToK8sJobMap := map[string]*kbatch.Job{}
for _, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob
for i, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
}

jobIDToPodsMap := map[string][]kcore.Pod{}
Expand Down Expand Up @@ -229,8 +229,8 @@ func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResp
}

jobIDToK8sJobMap := map[string]*kbatch.Job{}
for _, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &kJob
for i, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
}

endpoint, err := operator.APIEndpoint(api)
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/resources/job/batchapi/cron.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ func checkForJobFailure(jobKey spec.JobKey, k8sJob kbatch.Job) (bool, error) {

reasonFound := false
pods, _ := config.K8s.ListPodsByLabel("jobID", jobKey.ID)
for _, pod := range pods {
for i := range pods {
pod := pods[i] // to avoid loop pointer bugs
if k8s.WasPodOOMKilled(&pod) {
jobLogger.Error("at least one worker was killed because it ran out of out of memory")
return true, errors.FirstError(
Expand Down
10 changes: 5 additions & 5 deletions pkg/operator/resources/job/taskapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
taskAPIsMap := map[string]*schema.APIResponse{}

jobIDToK8sJobMap := map[string]*kbatch.Job{}
for _, job := range k8sJobs {
jobIDToK8sJobMap[job.Labels["jobID"]] = &job
for i, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
}

jobIDToPodsMap := map[string][]kcore.Pod{}
Expand Down Expand Up @@ -204,7 +204,7 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService, k8sJobs
return taskAPIList, nil
}

// GetAllAPIs returns a single task API and its most recently submitted job along with all running task jobs
// GetAPIByName returns a single task API and its most recently submitted job along with all running task jobs
func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResponse, error) {
virtualService := deployedResource.VirtualService

Expand All @@ -220,8 +220,8 @@ func GetAPIByName(deployedResource *operator.DeployedResource) ([]schema.APIResp
}

jobIDToK8sJobMap := map[string]*kbatch.Job{}
for _, job := range k8sJobs {
jobIDToK8sJobMap[job.Labels["jobID"]] = &job
for i, kJob := range k8sJobs {
jobIDToK8sJobMap[kJob.Labels["jobID"]] = &k8sJobs[i]
}

endpoint, err := operator.APIEndpoint(api)
Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/resources/job/taskapi/cron.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ func reconcileInProgressJob(jobState *job.State, jobFound bool) (status.JobCode,

func checkIfJobCompleted(jobKey spec.JobKey, k8sJob kbatch.Job) error {
pods, _ := config.K8s.ListPodsByLabel("jobID", jobKey.ID)
for _, pod := range pods {
if k8s.WasPodOOMKilled(&pod) {
for i := range pods {
if k8s.WasPodOOMKilled(&pods[i]) {
return errors.FirstError(
job.SetWorkerOOMStatus(jobKey),
deleteJobRuntimeResources(jobKey),
Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/resources/job/worker_stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ func GetWorkerCountsForJob(k8sJob kbatch.Job, pods []kcore.Pod) status.WorkerCou
}

workerCounts := status.WorkerCounts{}
for _, pod := range pods {
addPodToWorkerCounts(&pod, &workerCounts)
for i := range pods {
addPodToWorkerCounts(&pods[i], &workerCounts)
}

return workerCounts
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/resources/realtimeapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ func GetAllAPIs(pods []kcore.Pod, deployments []kapps.Deployment) ([]schema.APIR

realtimeAPIs := make([]schema.APIResponse, len(apis))

for i, api := range apis {
for i := range apis {
api := apis[i]
endpoint, err := operator.APIEndpoint(&api)
if err != nil {
return nil, err
Expand Down
11 changes: 6 additions & 5 deletions pkg/operator/resources/realtimeapi/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ func GetStatus(apiName string) (*status.Status, error) {

func GetAllStatuses(deployments []kapps.Deployment, pods []kcore.Pod) ([]status.Status, error) {
statuses := make([]status.Status, len(deployments))
for i, deployment := range deployments {
status, err := apiStatus(&deployment, pods)
for i := range deployments {
st, err := apiStatus(&deployments[i], pods)
if err != nil {
return nil, err
}
statuses[i] = *status
statuses[i] = *st
}

sort.Slice(statuses, func(i, j int) bool {
Expand Down Expand Up @@ -96,11 +96,12 @@ func getReplicaCounts(deployment *kapps.Deployment, pods []kcore.Pod) status.Rep
counts := status.ReplicaCounts{}
counts.Requested = *deployment.Spec.Replicas

for _, pod := range pods {
for i := range pods {
pod := pods[i]
if pod.Labels["apiName"] != deployment.Labels["apiName"] {
continue
}
addPodToReplicaCounts(&pod, deployment, &counts)
addPodToReplicaCounts(&pods[i], deployment, &counts)
}

return counts
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/resources/trafficsplitter/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ func GetAllAPIs(virtualServices []istioclientnetworking.VirtualService) ([]schem
return nil, err
}

for _, trafficSplitter := range apis {
for i := range apis {
trafficSplitter := apis[i]
endpoint, err := operator.APIEndpoint(&trafficSplitter)
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/resources/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,8 @@ func awsManagedValidateK8sCompute(compute *userconfig.Compute, maxMemMap map[str
}

func validateEndpointCollisions(api *userconfig.API, virtualServices []istioclientnetworking.VirtualService) error {
for _, virtualService := range virtualServices {
for i := range virtualServices {
virtualService := virtualServices[i]
gateways := k8s.ExtractVirtualServiceGateways(&virtualService)
if !gateways.Has("apis-gateway") {
continue
Expand Down